Re: ltdl bugs

[Jeff Squyres]

On Apr 3, 2005, at 10:17 AM, Ralf Wildenhues wrote:

> > I tried moving the three LT_DLFREE after the brace, and it /seems/ ok
> > then.  Am quite reluctant to put it in yet, because I fear to have
> > overlooked something.

So actually, I didn't do that for a specific reason: they're NULL in 
the self case.

Granted -- I only tested this on Linux; it's quite possible that 
they're not NULL in other cases.   Easy to move these FREE's beyond the 
brace and protect them in "if" statements.

> Ah, yes, here's the argument:
>
> When we accept your patch, Jeff, we allow to let ltdl's view of loaded
> modules get out of sync with what is really loaded.  Now promise me 
> that
> we don't currently use any system API which disallows opening of an
> already opened module.  Nor that we will ever implement support for 
> such
> an API.
>
> The general idea:
>   lt_dlopen()
>   lt_dlmakeresident()
>   lt_dlclose()
>   /* later.. */
>   lt_dlopen(same_module)
>
> I don't feel competent to judge on this assumption.
>
> Note it might be possible to do what you want inside lt_dlexit(), but
> the amount of hair would grow.

Hmm.  I'm not sure I understand this.  Doesn't "resident" mean that the 
module is statically compiled in the application (i.e., no underlying 
dlopen() [or whatever] was required to load it)?  If so:

- we're talking about the pre-sym case: presym_close() is a no-op, so 
it is safe to call this function, even in the self or pre-loaded case.

- we're also talking about the self case: on Linux, that's 
sys_dl_open(), which calls dlopen() with a filename==NULL (and gets a 
handle back).  Technically, we should call dlclose() with that handle, 
too -- but the man page (on Linux) doesn't specify what happens in that 
scenario.  Some tests on Linux and Solaris seem to show that this is ok 
(which makes sense -- if dlopen() allows you to open NULL, then it must 
allow you to close it, too).  So it is safe to call this function for 
the self case.

- also, ltdl's view of loaded modules is out of sync with what is 
really loaded until you call lt_dlopen() the first time.

So my [ammended, per above] patch simply resets ltdl's view of what is 
loaded to what it was before you called lt_dlopen().

Is that correct?  If so, my patch actually becomes much simpler -- 
there's no need to cordon off the unload module stuff -- just protect 
the DLFREE's with if statements:

-----
--- ltdl.c.orig 2005-04-03 08:49:11.000000000 -0400
+++ ltdl.c      2005-04-03 10:53:16.000000000 -0400
@@ -3815,10 +3815,19 @@
       errors += unload_deplibs(handle);

       /* It is up to the callers to free the data itself.  */
-      LT_DLFREE (handle->caller_data);
-
-      LT_DLFREE (handle->info.filename);
-      LT_DLFREE (handle->info.name);
+      if (NULL != handle->caller_data)
+        {
+          LT_DLFREE (handle->caller_data);
+        }
+
+      if (NULL != handle->info.filename)
+        {
+          LT_DLFREE (handle->info.filename);
+        }
+      if (NULL != handle->info.name)
+        {
+          LT_DLFREE (handle->info.name);
+        }
       LT_DLFREE (handle);

       goto done;
-----

Or I'm missing something...?

--
{+} Jeff Squyres
{+} address@hidden
{+} http://www.lam-mpi.org/