Re: cygwin dlopening backends

[Ralf Wildenhues]

Hi Eric,

First, let me thank you for excellent and quick review!

* Eric Blake wrote on Sun, Nov 13, 2005 at 05:50:31AM CET:
> Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes:
> > 
> > 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().

> 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.

I'll do anything like that only when pushed to.  :)
But thanks for mentioning.

> > Also, I wasn't sure whether paths on w32 (all incarnations) can be bound
> > by MAX_PATH.
> 
> 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. 

ACK.

> 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.

Ah, thanks for explaining this.  Surely it's a good idea not to waste
lots of stack then.  We're not time-critical anyway, malloc is fine.

*snip*

Regarding your review:

> > +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.

Good point.  Changed: loadlibrary.c is internal, so we do not need to
rely on public interface specs.

> > +  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.

Fixed.

> > +      if (LT_STRLEN (filename) >= MAX_PATH)
> 
> Should you cache the length here?...
*snip*

> > +   {
> > +     /* Append a `.' to stop Windows from adding an
> > +        implicit `.dll' extension. */
> > +     searchname = MALLOC (char, 2+ LT_STRLEN (wpath));
> 
> ...to avoid recalculating it here?

Yes, in some cases.  The actual semantics are a bit tricky: when we
want to append the dot, and we have done the cygwin path conversion,
we do need to recalculate the length.  See below.

> > +     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?

Reusing wpath seems fine, as does not using sprintf. :)

Updated, completely untested patch, except for a quick cross-compile
below (I'll do another w32 round in a couple of days, probably).

Beside addressing your comments, the patch now also sets the errors when
file names are too long, and the check is a bit stricter, too.

Cheers,
Ralf

2005-11-13  Eric Blake  <address@hidden>,
            Ralf Wildenhues  <address@hidden>

        * libltdl/ltdl.h, libltdl/ltdl.c (lt_dlinterface_free): New
        function.
        * doc/libtool.texi (User defined module data): Document it.

        * libltdl/loaders/loadlibrary.c (iface_id): New variable.
        (get_vtable): Get an `iface_id' from `lt_dlinterface_register'.
        (get_vtable): Rewrite to catch up with lt_dlhandle_iterate
        interface change.  Append dot only after w32 path conversion
        so it works on cygwin managed mounts.

Index: doc/libtool.texi
===================================================================
RCS file: /cvsroot/libtool/libtool/doc/libtool.texi,v
retrieving revision 1.204
diff -u -r1.204 libtool.texi
--- doc/libtool.texi    7 Nov 2005 14:35:35 -0000       1.204
+++ doc/libtool.texi    13 Nov 2005 22:01:26 -0000
@@ -3848,6 +3848,10 @@
 returned by the iteration functions below.
 @end deftypefun
 
address@hidden void lt_dlinterface_free (@w{lt_dlinterface_id @var{iface}})
+Release the data associated with @var{iface}.
address@hidden deftypefun
+
 @deftypefun int lt_dlhandle_map (@w{lt_dlinterface_id @var{iface}}, @w{int 
(address@hidden) (lt_dlhandle @var{handle}, void * @var{data})}, @w{void * 
@var{data}})
 For each module that matches @var{iface}, call the function
 @var{func}.  When writing the @var{func} callback function, the
Index: libltdl/ltdl.c
===================================================================
RCS file: /cvsroot/libtool/libtool/libltdl/ltdl.c,v
retrieving revision 1.237
diff -u -r1.237 ltdl.c
--- libltdl/ltdl.c      13 Nov 2005 21:59:54 -0000      1.237
+++ libltdl/ltdl.c      13 Nov 2005 22:01:26 -0000
@@ -2028,12 +2028,19 @@
     {
       interface_id->id_string = lt__strdup (id_string);
       if (!interface_id->id_string)
-       FREE (interface_id);
+        FREE (interface_id);
       else
-       interface_id->iface = iface;
+        interface_id->iface = iface;
     }
 
   return (lt_dlinterface_id) interface_id;
+}
+
+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);
 }
 
 void *
Index: libltdl/ltdl.h
===================================================================
RCS file: /cvsroot/libtool/libtool/libltdl/ltdl.h,v
retrieving revision 1.82
diff -u -r1.82 ltdl.h
--- libltdl/ltdl.h      26 Oct 2005 10:26:48 -0000      1.82
+++ libltdl/ltdl.h      13 Nov 2005 22:01:27 -0000
@@ -111,6 +111,7 @@
 
 LT_SCOPE lt_dlinterface_id lt_dlinterface_register (const char *id_string,
                                          lt_dlhandle_interface *iface);
+LT_SCOPE void  lt_dlinterface_free (lt_dlinterface_id key);
 LT_SCOPE void *        lt_dlcaller_set_data  (lt_dlinterface_id key,
                                          lt_dlhandle handle, void *data);
 LT_SCOPE void *        lt_dlcaller_get_data  (lt_dlinterface_id key,
Index: libltdl/loaders/loadlibrary.c
===================================================================
RCS file: /cvsroot/libtool/libtool/libltdl/loaders/loadlibrary.c,v
retrieving revision 1.9
diff -u -r1.9 loadlibrary.c
--- libltdl/loaders/loadlibrary.c       23 Sep 2005 07:58:42 -0000      1.9
+++ libltdl/loaders/loadlibrary.c       13 Nov 2005 22:01:29 -0000
@@ -50,6 +50,8 @@
 static void *   vm_sym   (lt_user_data loader_data, lt_module module,
                          const char *symbolname);
 
+static lt_dlinterface_id iface_id = 0;
+
 /* Return the vtable for this loader, only the name and sym_prefix
    attributes (plus the virtual function implementations, obviously)
    change between loaders.  */
@@ -61,6 +63,7 @@
   if (!vtable)
     {
       vtable = lt__zalloc (sizeof *vtable);
+      iface_id = lt_dlinterface_register ("ltdl loadlibrary", NULL);
     }
 
   if (vtable && !vtable->name)
@@ -96,37 +100,52 @@
 vm_open (lt_user_data loader_data, const char *filename)
 {
   lt_module    module     = 0;
-  char        *searchname = 0;
-  char        *ext;
-  char         self_name_buf[MAX_PATH];
+  char         *ext;
+  char         wpath[MAX_PATH];
+  size_t       len;
 
   if (!filename)
     {
       /* Get the name of main module */
-      *self_name_buf = 0;
-      GetModuleFileName (NULL, self_name_buf, sizeof (self_name_buf));
-      filename = ext = self_name_buf;
+      *wpath = 0;
+      GetModuleFileName (NULL, wpath, sizeof (wpath));
+      filename = wpath;
     }
   else
     {
-      ext = strrchr (filename, '.');
-    }
+      len = LT_STRLEN (filename);
 
-  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 (len >= MAX_PATH)
+        {
+         LT__SETERROR (CANNOT_OPEN);
+         return 0;
+       }
+
+#if defined(__CYGWIN__)
+      cygwin_conv_to_full_win32_path (filename, wpath);
+      len = 0;
+#else
+      strcpy(wpath, filename);
+#endif
+
+      ext = strrchr (wpath, '.');
+      if (!ext)
+       {
+         /* Append a `.' to stop Windows from adding an
+            implicit `.dll' extension. */
+         if (!len)
+           len = LT_STRLEN (wpath);
+
+         if (len + 1 >= MAX_PATH)
+           {
+             LT__SETERROR (CANNOT_OPEN);
+             return 0;
+           }
+
+         wpath[len] = '.';
+         wpath[len+1] = '\0';
+       }
     }
-  if (!searchname)
-    return 0;
 
   {
     /* Silence dialog from LoadLibrary on some failures.
@@ -135,20 +154,11 @@
     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
+    module = LoadLibrary (wpath);
 
     /* Restore the error mode. */
     SetErrorMode(errormode);
   }
-  FREE (searchname);
 
   /* libltdl expects this function to fail if it is unable
      to physically load the library.  Sadly, LoadLibrary
@@ -161,20 +171,20 @@
   {
     lt__handle *        cur        = 0;
 
-    while ((cur = (lt__handle *) lt_dlhandle_next ((lt_dlhandle) cur)))
+    while ((cur = (lt__handle *) lt_dlhandle_iterate (iface_id, (lt_dlhandle) 
cur)))
       {
         if (!cur->module)
           {
             cur = 0;
             break;
           }
-        
+
         if (cur->module == module)
           {
             break;
           }
       }
-    
+
     if (cur || !module)
       {
         LT__SETERROR (CANNOT_OPEN);