Re: About the dll search algorithm of dlopen (patch-r2)

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

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.

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

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

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

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

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


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]