Re: cygwin dlopening backends

[Eric Blake]

Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes:

> 
> Hi Charles, Eric,

Hi Ralf,

> 
> * Charles Wilson wrote on Thu, Nov 10, 2005 at 05:55:28AM CET:
> > Ralf Wildenhues wrote:
> > >
> > >There are several separate issues here:
> > >
> > >1) lt_dlhandle_iterate breakage of loadlibrary.c
> > >2) needed dlinterface_free (or maybe _unregister?)
> > >   including documentation
> > >3) cygwin managed mount fix of loadlibrary.c
> > >   or remove the cygwin-specific code of loadlibrary.c
> > >4) use either
> > >    - only dlopen, or
> > >    - first dlopen, then LoadLibrary
> > >   on cygwin, or
> > >    - make the choice configurable
> >
> > I think Ralf's issues (1) and (2) need fixing first
> 
> Please take a look at and test the following patches which should
> address (1), (2), and (3).  I have not done a lot testing myself /yet/,
> so beware.

In general, it looks good to me.  Minor nits below.  I'll try running it through
the full testsuite, but it may take me a while.

> 
> I'm not so sure whether we should register/free the thing in loadlibrary
> every time instead of once at the start: those memory checker users
> always go nuts when they find a small, constant-amount of allocated
> memory not freed before exit().
> (OTOH, we might be lucky in that there aren't any good checkers -- at
> least that I know of -- for mingw or cygwin 

I'm fine with your approach of creating it just once, because you maintain a
handle to it.  If you are really worried about a memory leak, use an atexit()
function to clean up after yourself.

> 
> Also, I wasn't sure whether paths on w32 (all incarnations) can be bound
> by MAX_PATH.  The documentation for cygwin_conv_to_full_win32_path
> suggests that at least for cygwin this is safe.

Yes, the cygwin_conv* functions are bounded by MAX_PATH.  I would much rather
have an interface that does not have an implicit, arbitrary length (preferring
either the user passes in the max length of a pre-allocated buffer, or the
function itself does the malloc), but that was not done in this case. 
Currently, MAX_PATH on cygwin is 256 (okay by strict POSIX but not by XSI
standards) because of corresponding limits in the ASCII versions of Windows
syscalls.  There is talk (but just that, because it would be a huge patch,
particularly while maintaining backward compatibility) of switching cygwin to
use the Unicode version of Windows syscalls on NT machines (with suitable
fallbacks for the 9x family which only supports ASCII), at which point MAX_PATH
would be increased to a more reasonable 32k on NT.

> 
> +void lt_dlinterface_free (lt_dlinterface_id key)
> +{
> +  lt__interface_id *interface_id = (lt__interface_id *)key;
> +  FREE (interface_id->id_string);
> +  FREE (interface_id);
> +}

This made me realize that there is another problem with lt_dlinterface_register:

2005-10-26  Eric Blake  <address@hidden>

        * libltdl/ltdl.c (lt_dlinterface_register): Fail if lt__strdup did.

Index: libltdl/ltdl.c
===================================================================
RCS file: /cvsroot/libtool/libtool/libltdl/ltdl.c,v
retrieving revision 1.236
diff -u -p -r1.236 ltdl.c
--- libltdl/ltdl.c      26 Oct 2005 10:26:48 -0000      1.236
+++ libltdl/ltdl.c      13 Nov 2005 04:35:45 -0000
@@ -2027,7 +2027,10 @@ lt_dlinterface_register (const char *id_
   if (interface_id)
     {
       interface_id->id_string = lt__strdup (id_string);
-      interface_id->iface = iface;
+      if (!interface_id->id_string)
+       FREE (interface_id);
+      else
+       interface_id->iface = iface;
     }

   return (lt_dlinterface_id) interface_id;


> +static lt_dlinterface_id iface_id = 0;
> +
> +static int
> +loadlibrary__module_interface (lt_dlhandle handle, const char *id_string)
> +{
> +  /* we _need_ to look at every module, so pretend all belong to us */
> +  return 0;
> +}

Hmm, you are actually registering a no-op function, even though
lt_dlhandle_iterate behaves the same (although slightly faster) had you
registered a NULL pointer instead as in my original version of the patch.  But I
guess that is okay, in case we ever change lt_dlinterface_register to require a
non-NULL function.

>  <at>  <at>  -99,6 +110,7  <at>  <at> 
>    char              *searchname = 0;
>    char              *ext;
>    char               self_name_buf[MAX_PATH];
> +  char               wpath[MAX_PATH];

If cygwin ever increases MAX_PATH beyond 256, self_name_buf and wpath together
would become a huge burden on the stack.

> 
>    if (!filename)
>      {
>  <at>  <at>  -109,24 +121,33  <at>  <at> 
>      }
>    else
>      {
> -      ext = strrchr (filename, '.');
> -    }
> +      if (LT_STRLEN (filename) >= MAX_PATH)

Should you cache the length here?...

> +     return 0;
> 
> -  if (ext)
> -    {
> -      /* FILENAME already has an extension. */
> -      searchname = lt__strdup (filename);
> -    }
> -  else
> -    {
> -      /* Append a `.' to stop Windows from adding an
> -      implicit `.dll' extension. */
> -      searchname = MALLOC (char, 2+ LT_STRLEN (filename));
> -      if (searchname)
> -     sprintf (searchname, "%s.", filename);
> +#if defined(__CYGWIN__)
> +      cygwin_conv_to_full_win32_path (filename, wpath);
> +#else
> +      strcpy(wpath, filename);
> +#endif
> +
> +      ext = strrchr (wpath, '.');
> +
> +      if (ext)
> +     {
> +       /* FILENAME already has an extension. */
> +       searchname = lt__strdup (wpath);
> +     }
> +      else
> +     {
> +       /* Append a `.' to stop Windows from adding an
> +          implicit `.dll' extension. */
> +       searchname = MALLOC (char, 2+ LT_STRLEN (wpath));

...to avoid recalculating it here?

> +       if (searchname)
> +         sprintf (searchname, "%s.", wpath);

And since you are already limited by MAX_PATH, and already have wpath that
large, do you really need to malloc searchname, or could you just reuse wpath?

> +     }
> +      if (!searchname)
> +     return 0;
>      }
> -  if (!searchname)
> -    return 0;
> 
>    {
>      /* Silence dialog from LoadLibrary on some failures.
>  <at>  <at>  -135,15 +156,7  <at>  <at> 
>      UINT errormode = SetErrorMode(SEM_FAILCRITICALERRORS);
>      SetErrorMode(errormode | SEM_FAILCRITICALERRORS);
> 
> -#if defined(__CYGWIN__)
> -    {
> -      char wpath[MAX_PATH];
> -      cygwin_conv_to_full_win32_path (searchname, wpath);
> -      module = LoadLibrary (wpath);
> -    }
> -#else
>      module = LoadLibrary (searchname);
> -#endif
> 
>      /* Restore the error mode. */
>      SetErrorMode(errormode);


--
Eric Blake