About the dll search algorithm of dlopen (patch-r3)

[Michael Haubenwallner]

Hi Corinna,

On 08/22/2016 08:37 PM, Corinna Vinschen wrote:
> Hi Michael,
> 
> On Aug 22 18:02, Michael Haubenwallner wrote:
>> What about this one: 've dropped any templates now,
>> while keeping the basenames logic inside dlopen.
> 
> I was going to reply to your other mail but I'll deferred that for now
> and looked into your code instead.  I think we can basically go along
> with this.  A few points, though.

besides addressing your concerns I've also split the patch one more time:
1) switch dlopen to pathfinder, not changing search behaviour yet
2) fix pathfinder to search basenames within single dir

> 
>> +      /* Finally we better have some fallback. */
>> +      finder.add_searchdir ("/usr/lib", -1);
> 
> You're not adding /usr/bin here because your code contains automatic
> code for lib -> bin duplication... see (*) below.
> 
>> @@ -113,6 +103,7 @@ dlopen (const char *name, int flags)
>>  {
>>    void *ret = NULL;
>>  
>> +  debug_printf ("flags %d for %s", flags, name);
> 
> Stray debug_printf?  Otherwise, if you have a reason to see the
> flags, please provide an extra patch for this.

've been more after the name passed to dlopen, actually.

> 
>> diff --git a/winsup/cygwin/pathfinder.h b/winsup/cygwin/pathfinder.h
>> new file mode 100644
>> index 0000000..3453692
>> --- /dev/null
>> +++ b/winsup/cygwin/pathfinder.h
>> @@ -0,0 +1,112 @@
>> +/* pathfinder.h: find one of multiple file names in path
>> +
>> +   Copyright 2016 Red Hat, Inc.
> 
> Minor point:  Please drop the "Copyright ..., Red Hat" line.  With the
> new LGPL copyright regime we removed them.  They are not required and
> keeping them in shape is awkward.

ok.

> 
>> +      /* Search "x/bin:x/lib" for "x/lib" */
>> +      if (dirlen >=4 && !strncmp (dir + dirlen - 4, "/lib", 4))
>> +	/* prealloc buffer in searchdir for any basename we will search for */
>> +	searchbuffers_.appendv (dir, dirlen - 4, "/bin", 4, "/", 1 + basenames_maxlen_, NULL);
>> +
>> +      /* prealloc buffer in searchdir for any basename we will search for */
>> +      searchbuffers_.appendv (dir, dirlen, "/", 1 + basenames_maxlen_, NULL);
>> +  }
> 
> (*) Yuk!  Do we really, *really* want that?  The redirection from
>     /usr/lib to /usr/bin is only done for system libs, and only because
>     otherwise we had trouble starting Cygwin from CMD or the Explorer
>     GUI "Run..." box.  We can't change this without breaking everything
>     since we *do* depend on the Windows loader yet.
>     
>     However, as long as this is restricted to /usr/lib, /usr/bin, it's a
>     closed system under control of "the distro".  If you extend this to
>     *any* external path ending in "lib", isn't it inherently dangerous
>     to automate this under the hood, without the application's consent?
>     Or, FWIW, the user's consent in case of LD_LIBRARY_PATH?

've split into add_lib_searchdir (), used for "/usr/lib" only.

Should be a separate patch anyway if really necessary.

> 
>> diff --git a/winsup/cygwin/vstrlist.h b/winsup/cygwin/vstrlist.h
>> new file mode 100644
>> index 0000000..ecbdc64
>> --- /dev/null
>> +++ b/winsup/cygwin/vstrlist.h
>> [...]
>> +    void * operator new (size_t class_size, char const * part0, va_list parts)
>> +    {
>> +      char const * part = part0;
>> +      va_list partsdup;
>> +      va_copy (partsdup, parts);
>> +      size_t bufferlength = 0;
>> +      while (part)
>> +	{
>> +	  int partlen = va_arg (partsdup, int);
>> +	  if (partlen < 0)
>> +	    partlen = strlen (part);
>> +	  bufferlength += partlen;
>> +	  part = va_arg (partsdup, char const *);
>> +	}
>> +      va_end (partsdup);
>> +
>> +      return cmalloc_abort (HEAP_STR, class_size + bufferlength);
>> +    }
> 
> Uhm... I don't think this is correct.  Using cmalloc and friends has a
> specific purpose.  Only internal datastructures which are inherited by
> child processes via fork/exec are supposed to be allocated on the
> cygheap.  The cygheap really shouldn't be used freely for other
> purposes.  You could leak it to child processes if another thread forks
> during your thread's call to dlopen.  Also, especially on 32 bit the
> pressure on the cygheap is not insignificant.  Please use malloc/free
> instead.

Accepted - wasn't aware of cygheap's dedication to fork only.

Using tmp_pathbuf now, wrapped behind some trivial allocator - which
might fit better somewhere else than to dlfcn.cc?

BTW: Is it really intended for tmp_pathbuf to have a single active
instance (per thread) at a time?

> 
> Other than that your patch looks pretty well to me.  Except, uhm...
> maybe you want to comment the new classes with a usage description?

Done (tried at least).

Thanks!
/haubi/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dlopen-switch-to-new-pathfinder-class.patch
Type: text/x-patch
Size: 20066 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-developers/attachments/20160825/7e34a8af/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-dlopen-pathfinder-try-each-basename-per-dir.patch
Type: text/x-patch
Size: 1364 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-developers/attachments/20160825/7e34a8af/attachment-0001.bin>