About the dll search algorithm of dlopen

[Michael Haubenwallner]

Hi,

two issues with dlopen here (I'm about to prepare patches):

*) The algorithm to combine dll file name variants with the search path
entries needs to be reordered, as in:
- for each dll file name variant:
- for each search path:
+ for each search path entry:
+ for each dll file name variant:
check if useable

*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

For consistency, IMO, when any searched path ends in either
x/bin or x/lib, we should search x/bin:x/lib.

Thoughts?

Thanks!
/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

Rationale? We only need to find one version of the file and there
usually only is one. This is mainly for moduled systems like perl,
python, etc. However, if you can speed up the search process ignore the
question...

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Having said that, LoadLibrary will search the usual paths. After 2.5.2,
we're leaving XP/2003 behind, and then we probably should tighten the
search algorithm along the lines of

AddDllDirectory ("/usr/bin");
AddDllDirectory ("/usr/lib");
[...]
LoadLibraryEx (path, NULL, LOAD_LIBRARY_SEARCH_USER_DIRS
| LOAD_LIBRARY_SEARCH_SYSTEM32);

Post by Michael Haubenwallner
For consistency, IMO, when any searched path ends in either
x/bin or x/lib, we should search x/bin:x/lib.

This might make sense, at least in the direction lib->bin.


Corinna

[Michael Haubenwallner]

Post by Corinna Vinschen

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

Rationale? We only need to find one version of the file and there
usually only is one. This is mainly for moduled systems like perl,
python, etc.

While I indeed didn't face a problem here yet, the rationale behind is
that I need to install a large application that provides its own (portable)
package build & management system, including lots of packages probably installed
in the host (Cygwin) system as well, most likely in (slightly) different versions.

When these package management system now does provide a different dll naming
scheme than the host system, like stick to "liblib.dll" rather than "cyglib.dll",
and the application wants to dlopen its own "liblib.dll", currently the host's
"cyglib.dll" is loaded.

Post by Corinna Vinschen
However, if you can speed up the search process ignore the
question...

This also depends on whether find_exec really is necessary here.

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.
However, there is one path name that can easily serve as minimal
"embedded RUNPATH" - the executable's directory.

This is where I do have a problem right now:

My own /application/bin/python2.7.exe is linked to libpython2.7.dll,
located in /application/bin. Now there is some python script that does
have some - strange enough - cygwin-conditional code that reads:

import _ctypes
_ctypes.dlopen("libpython%d.%d.dll" % sys.version_info[:2])

While this is questionable by itself, it really shouldn't load another
libpython2.7.dll than /application/bin/python2.7.exe has already loaded
just because dlopen using a different search algorithm than CreateProcess().

However, when dlopen is about to search some path list, I can imagine to
search the list of already loaded dlls first as well, but I'd prefer to
leave this up to LoadLibrary...

Post by Corinna Vinschen
Having said that, LoadLibrary will search the usual paths. After 2.5.2,
we're leaving XP/2003 behind, and then we probably should tighten the
search algorithm along the lines of
AddDllDirectory ("/usr/bin");
AddDllDirectory ("/usr/lib");
[...]
LoadLibraryEx (path, NULL, LOAD_LIBRARY_SEARCH_USER_DIRS
| LOAD_LIBRARY_SEARCH_SYSTEM32);

/me fails to see how this does help with the missing embedded RUNPATH.

Post by Corinna Vinschen

Post by Michael Haubenwallner
For consistency, IMO, when any searched path ends in either
x/bin or x/lib, we should search x/bin:x/lib.

This might make sense, at least in the direction lib->bin.

Fine with me too.

Side note:
We also use some cl.exe/link.exe wrapper that supports LD_PRELOAD,
LD_LIBRARY_PATH, embedded RUNPATH, as well as lazy loading for both
LoadLibrary and CreateProcess: https://github.com/haubi/parity
Basically I'm wondering why Cygwin doesn't provide that (yet?)...

/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

Rationale? We only need to find one version of the file and there
usually only is one. This is mainly for moduled systems like perl,
python, etc.

While I indeed didn't face a problem here yet, the rationale behind is
that I need to install a large application that provides its own (portable)
package build & management system, including lots of packages probably installed
in the host (Cygwin) system as well, most likely in (slightly) different versions.
When these package management system now does provide a different dll naming
scheme than the host system, like stick to "liblib.dll" rather than "cyglib.dll",
and the application wants to dlopen its own "liblib.dll", currently the host's
"cyglib.dll" is loaded.

Sounds a teeny bit artificial, but basically that's possible, yes.

Post by Michael Haubenwallner

Post by Corinna Vinschen
However, if you can speed up the search process ignore the
question...

This also depends on whether find_exec really is necessary here.

Not as such necessary, it's just the function used to search in a
search path. If you want to change that you have to rewrite the
same logic again, just reversed.

One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

Post by Michael Haubenwallner
However, there is one path name that can easily serve as minimal
"embedded RUNPATH" - the executable's directory.
My own /application/bin/python2.7.exe is linked to libpython2.7.dll,
located in /application/bin. Now there is some python script that does
import _ctypes
_ctypes.dlopen("libpython%d.%d.dll" % sys.version_info[:2])
While this is questionable by itself, it really shouldn't load another
libpython2.7.dll than /application/bin/python2.7.exe has already loaded
just because dlopen using a different search algorithm than CreateProcess().
However, when dlopen is about to search some path list, I can imagine to
search the list of already loaded dlls first as well, but I'd prefer to
leave this up to LoadLibrary...

This problem would only occur if dlopen is not called with a path. If
the given pathname is a plain filename, we could simply add a call to
GetModuleHandle and if it succeeds, return the handle (after checking
for RTLD_NODELETE).

Post by Michael Haubenwallner

Post by Corinna Vinschen
Having said that, LoadLibrary will search the usual paths. After 2.5.2,
we're leaving XP/2003 behind, and then we probably should tighten the
search algorithm along the lines of
AddDllDirectory ("/usr/bin");
AddDllDirectory ("/usr/lib");
[...]
LoadLibraryEx (path, NULL, LOAD_LIBRARY_SEARCH_USER_DIRS
| LOAD_LIBRARY_SEARCH_SYSTEM32);

/me fails to see how this does help with the missing embedded RUNPATH.

It doesn't. It just tightens the search path to not load from the cwd
or the application path. If you want that, add it to LD_LIBRARY_PATH
explicitely.

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
For consistency, IMO, when any searched path ends in either
x/bin or x/lib, we should search x/bin:x/lib.

This might make sense, at least in the direction lib->bin.

Fine with me too.
We also use some cl.exe/link.exe wrapper that supports LD_PRELOAD,
LD_LIBRARY_PATH, embedded RUNPATH, as well as lazy loading for both
LoadLibrary and CreateProcess: https://github.com/haubi/parity
Basically I'm wondering why Cygwin doesn't provide that (yet?)...

We discussed implementing our own dynamic loader once, but gave up due
to workload. Parity is LGPLed and thus can't be included into Cygwin
itself.

DT_RUNTIME should be possible with not too much effort, but would
require gcc/binutils support. Some tricking with a symbol in the linker
script, setting a pointer to it in _cygwin_crt0_common, and a matching
call to AddDllDirectory comes to mind...

LD_PRELOAD is (kind of) implemented but I think doesn't work as
intended. Importing symbols is bound to the name of the DLL they came
from in a PE/COFF file. To implement the full set of ELF dynloader
features would require some major effort, like what parity does.


Corinna

[Yaakov Selkowitz]

Post by Corinna Vinschen
LD_PRELOAD is (kind of) implemented but I think doesn't work as
intended. Importing symbols is bound to the name of the DLL they came
from in a PE/COFF file.

However, as long as you're overriding symbols in cygwin1.dll,
cygwin_internal(CW_HOOK,...) allows LD_PRELOAD hacks to work as
intended. In fact, there are a few in the distro (artsdsp, datefudge,
pulsedsp, Xdummy).

[Michael Haubenwallner]

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

However, if you can speed up the search process ignore the
question...

Not sure if there's a speedup actually, ...

Post by Corinna Vinschen

Post by Michael Haubenwallner
This also depends on whether find_exec really is necessary here.

Not as such necessary, it's just the function used to search in a
search path. If you want to change that you have to rewrite the
same logic again, just reversed.
One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

... but:
0001.patch is a draft for some new cygwin::pathfinder class, with
0002.patch adding the executable's directory as searchpath, and
0003.patch to search the PATH environment as well.

Thoughts?
Any idea about different template nesting to be useful somewhere else?

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

In any case, to me it does feel quite important to have the (almost) same
dll search algorithm with dlopen() as with CreateProcess().

Post by Corinna Vinschen

Post by Michael Haubenwallner
However, when dlopen is about to search some path list, I can imagine to
search the list of already loaded dlls first as well, but I'd prefer to
leave this up to LoadLibrary...

This problem would only occur if dlopen is not called with a path. If
the given pathname is a plain filename, we could simply add a call to
GetModuleHandle and if it succeeds, return the handle (after checking
for RTLD_NODELETE).

But indeed this might lower the need for same search algorithms...

Post by Corinna Vinschen

Post by Michael Haubenwallner
We also use some cl.exe/link.exe wrapper that supports LD_PRELOAD,
LD_LIBRARY_PATH, embedded RUNPATH, as well as lazy loading for both
LoadLibrary and CreateProcess: https://github.com/haubi/parity
Basically I'm wondering why Cygwin doesn't provide that (yet?)...

We discussed implementing our own dynamic loader once, but gave up due
to workload. Parity is LGPLed and thus can't be included into Cygwin
itself.

've mentioned Parity more as proof-of-concept than for inclusion,
and accepting workload as reasoning.

Let's see if I can live without the ELF dynloader features.

Thanks!
/haubi/

[Michael Haubenwallner]

(and now with patches attached)

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

However, if you can speed up the search process ignore the
question...

Not sure if there's a speedup actually, ...

Post by Corinna Vinschen

Post by Michael Haubenwallner
This also depends on whether find_exec really is necessary here.

Not as such necessary, it's just the function used to search in a
search path. If you want to change that you have to rewrite the
same logic again, just reversed.
One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

0001.patch is a draft for some new cygwin::pathfinder class, with
0002.patch adding the executable's directory as searchpath, and
0003.patch to search the PATH environment as well.
Thoughts?
Any idea about different template nesting to be useful somewhere else?

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

In any case, to me it does feel quite important to have the (almost) same
dll search algorithm with dlopen() as with CreateProcess().

Post by Corinna Vinschen

Post by Michael Haubenwallner
However, when dlopen is about to search some path list, I can imagine to
search the list of already loaded dlls first as well, but I'd prefer to
leave this up to LoadLibrary...

This problem would only occur if dlopen is not called with a path. If
the given pathname is a plain filename, we could simply add a call to
GetModuleHandle and if it succeeds, return the handle (after checking
for RTLD_NODELETE).

But indeed this might lower the need for same search algorithms...

Post by Corinna Vinschen

Post by Michael Haubenwallner
We also use some cl.exe/link.exe wrapper that supports LD_PRELOAD,
LD_LIBRARY_PATH, embedded RUNPATH, as well as lazy loading for both
LoadLibrary and CreateProcess: https://github.com/haubi/parity
Basically I'm wondering why Cygwin doesn't provide that (yet?)...

We discussed implementing our own dynamic loader once, but gave up due
to workload. Parity is LGPLed and thus can't be included into Cygwin
itself.

've mentioned Parity more as proof-of-concept than for inclusion,
and accepting workload as reasoning.
Let's see if I can live without the ELF dynloader features.
Thanks!
/haubi/

[Corinna Vinschen]

Hi Michael,

Post by Michael Haubenwallner
(and now with patches attached)

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
Hi,
*) The algorithm to combine dll file name variants with the search path
check if useable

However, if you can speed up the search process ignore the
question...

Not sure if there's a speedup actually, ...

Post by Corinna Vinschen

Post by Michael Haubenwallner
This also depends on whether find_exec really is necessary here.

Not as such necessary, it's just the function used to search in a
search path. If you want to change that you have to rewrite the
same logic again, just reversed.
One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

0001.patch is a draft for some new cygwin::pathfinder class, with
0002.patch adding the executable's directory as searchpath, and
0003.patch to search the PATH environment as well.
Thoughts?

Ok, that might be disappointing now because you already put so much work
into it, but I actually expected some more discussion first. I have two
problem with this.

I'm not a big fan of templates. In fact, to me they are confusing and
hard to debug. We only use templates in a single instance in thread.cc
to implement linked lists of pthread objects and I would be very
grateful if we don't add to them. Somebody has to maintain the code,
and ultimately that would be me. We also don't really need namespaces
since we don't include any external C++ libs, so we don't have name
collisions. There are also a few style-issues in the code, e.g,
CamelBack. Cygwin usually doesn't use it (exceptions confirm the rule)
but it's usually a good way to differ Windows functions from Cygwin
functions.

What I had in mind was a *simple* class which gets told if it searches
for libs or executables and then checks the different paths accordingly,
kind of a copy of find_exec as a class, just additionally handling the
prefix issue for DLLs.

Post by Michael Haubenwallner

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

In any case, to me it does feel quite important to have the (almost) same
dll search algorithm with dlopen() as with CreateProcess().

Last but not least I'm not yet convinced if it's *really* a good idea to
prepend the executable path to the DLL search path unconditionally. Be
it as it is in terms of DT_RUNPATH, why is the application dir a good
choice at all, unless we're talking Windows semantics? Which we don't.
Also, if loading from the applications dir from dlopen is important for
you, you can emulate it by adding the application dir to LD_LIBRAYR_PATH.

I checked for the usage of DT_RUNPATH/DT_RPATH on Fedora 23 and only a
limited number of packages use it (texlive, samba, python, man-db,
swipl, and a few more). Some of them, like texlive, even use it wrongly,
with RPATH pointing to a non-existing build dir. There are also a few
stray "/usr/lib64" settings, but all in all it's not used to point to
the dir the application is installed to, but rather to some package specific
subdir, e.g. /usr/lib64/samba, /usr/lib64/swipl-7.2.3/lib/x86_64-linux,
etc.

IMHO this means just adding the applications bin dir is most of the time
an unused or even wrong workaround.


Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

0001.patch is a draft for some new cygwin::pathfinder class, with
0002.patch adding the executable's directory as searchpath, and
0003.patch to search the PATH environment as well.
Thoughts?

Ok, that might be disappointing now because you already put so much work
into it, but I actually expected some more discussion first. I have two
problem with this.
I'm not a big fan of templates.

Never mind, it's been some template exercise to me anyway.

Post by Corinna Vinschen
What I had in mind was a *simple* class which gets told if it searches
for libs or executables and then checks the different paths accordingly,
kind of a copy of find_exec as a class, just additionally handling the
prefix issue for DLLs.

What I'm more interested in for such a class is the actual API for use
by dlopen() and exec(), and the final list of files searched for - with
these use cases coming to my mind:

Libraries/dlls with final search path "/lib:/morelibs":
L1) dlopen("libN.so")
L2) dlopen("libN.dll")
L3) dlopen("cygN.dll")
L4) dlopen("N.so")
L5) dlopen("N.dll")
Executables with final search path "/bin:/moreexes"
X1) exec("X")
X2) exec("X.exe")
X3) exec("X.com")

Instead of API calls similar to:
L1) find(dll, "N", ["/lib", "/morelibs"])
L2) find(dll, "N", ["/lib", "/morelibs"])
L3) find(dll, "N", ["/lib", "/morelibs"])
L4) find(dll, "N", ["/lib", "/morelibs"])
L5) find(dll, "N", ["/lib", "/morelibs"])
X1) find(exe, "X", ["/bin", "/moreexes"])
X2) find(exe, "X", ["/bin", "/moreexes"])
X3) find(exe, "X", ["/bin", "/moreexes"])
it feels necessary to support more explicit naming, as in:
L1) find(["libN.so", "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L2) find([ "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L3) find([ "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L4) find(["N.so", "N.dll" ], ["/lib/../bin","/lib","/morelibs"])
L5) find([ "N.dll" ], ["/lib/../bin","/lib","/morelibs"])
X1) find(["X", "X.exe","X.com"], ["/bin","/moreexes"])
X2) find(["X", "X.exe" ], ["/bin","/moreexes"])
X3) find(["X", "X.com" ], ["/bin","/moreexes"])

Where the find method does not need to actually know whether it searches
for a dll or an exe, but dlopen() and exec() instead define the file
names to search for. This is what the patch draft does in dlopen.

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

In any case, to me it does feel quite important to have the (almost) same
dll search algorithm with dlopen() as with CreateProcess().

Last but not least I'm not yet convinced if it's *really* a good idea to
prepend the executable path to the DLL search path unconditionally. Be
it as it is in terms of DT_RUNPATH, why is the application dir a good
choice at all, unless we're talking Windows semantics? Which we don't.
Also, if loading from the applications dir from dlopen is important for
you, you can emulate it by adding the application dir to LD_LIBRAYR_PATH.

As long as there is lack of a Cygwin specific dll loader to find the
dlls to load during process startup, we're bound to Windows semantics.

For dlopen, it is more important to find the same dll file as would be
found when the exe was linked against that dll file, rather than using
the Linux-known algorithm and environment variables - and differ from
process startup: Both really should result in the same algorithm here,
even if that means some difference compared to Linux.

As far as I understand, lack of DT_RUNPATH (besides /etc/ld.so.conf)
support during process start was the main reason for the dlls to install
into /lib/../bin instead of /lib at all, to be found at process start
because of residing in the application's bin dir:
Why should that be different for dlopen?

Post by Corinna Vinschen
I checked for the usage of DT_RUNPATH/DT_RPATH on Fedora 23 and only a
limited number of packages use it (texlive, samba, python, man-db,
swipl, and a few more). Some of them, like texlive, even use it wrongly,
with RPATH pointing to a non-existing build dir. There are also a few
stray "/usr/lib64" settings, but all in all it's not used to point to
the dir the application is installed to, but rather to some package specific
subdir, e.g. /usr/lib64/samba, /usr/lib64/swipl-7.2.3/lib/x86_64-linux,
etc.

On Linux, the binaries installed in /usr usually rely on the Linux
loader to be configured via /etc/ld.so.conf to find their runtime
libs in /usr/lib.

Please remember: This whole thing is not a problem with packages
installed to /usr, but with packages installed to /somewhere/else
that provide runtime libraries that are also available in /usr.

Using LD_LIBRARY_PATH pointing to /somewhere/else/lib may break the
binaries found in /usr/bin - and agreed, searching PATH doesn't make
it better, as PATH is the "LD_LIBRARY_PATH" for Windows.

Post by Corinna Vinschen
IMHO this means just adding the applications bin dir is most of the time
an unused or even wrong workaround.

Although GetModuleHandle may reduce that pressure for dlopen - as long as
the applications bin dir is searched at process start, it really should
be searched by dlopen too, even if for /usr/bin/* this might indeed become
redundant, as we always add /usr/bin in dlopen - which really mimics
the /etc/ld.so.conf content actually, although that one is unavailable
to process startup.

Thanks!
/haubi/

[Peter Rosin]

This post might be inappropriate. Click to display it.

[Michael Haubenwallner]

Hi Peter,

Post by Peter Rosin
Just mentioning that changed dlopen semantics will in all likelihood affect
libtool. Do we have any libtool hackers around (besides me, I do not have
time) since Chuck disappeared?

/me has some (non-cygwin) libtool-patches@ pending for review&commit already...

/haubi/

[Michael Haubenwallner]

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
One way around YA code duplication could be some kind of path iterator
class which could be used from find_exec as well as from
get_full_path_of_dll.

0001.patch is a draft for some new cygwin::pathfinder class, with
0002.patch adding the executable's directory as searchpath, and
0003.patch to search the PATH environment as well.
Thoughts?

Ok, that might be disappointing now because you already put so much work
into it, but I actually expected some more discussion first. I have two
problem with this.
I'm not a big fan of templates.

Never mind, it's been some template exercise to me anyway.

Post by Corinna Vinschen
What I had in mind was a *simple* class which gets told if it searches
for libs or executables and then checks the different paths accordingly,
kind of a copy of find_exec as a class, just additionally handling the
prefix issue for DLLs.

What about this one: 've dropped any templates now,
while keeping the basenames logic inside dlopen.

/haubi/

Post by Michael Haubenwallner
What I'm more interested in for such a class is the actual API for use
by dlopen() and exec(), and the final list of files searched for - with
L1) dlopen("libN.so")
L2) dlopen("libN.dll")
L3) dlopen("cygN.dll")
L4) dlopen("N.so")
L5) dlopen("N.dll")
Executables with final search path "/bin:/moreexes"
X1) exec("X")
X2) exec("X.exe")
X3) exec("X.com")
L1) find(dll, "N", ["/lib", "/morelibs"])
L2) find(dll, "N", ["/lib", "/morelibs"])
L3) find(dll, "N", ["/lib", "/morelibs"])
L4) find(dll, "N", ["/lib", "/morelibs"])
L5) find(dll, "N", ["/lib", "/morelibs"])
X1) find(exe, "X", ["/bin", "/moreexes"])
X2) find(exe, "X", ["/bin", "/moreexes"])
X3) find(exe, "X", ["/bin", "/moreexes"])
L1) find(["libN.so", "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L2) find([ "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L3) find([ "cygN.dll", "libN.dll"], ["/lib/../bin","/lib","/morelibs"])
L4) find(["N.so", "N.dll" ], ["/lib/../bin","/lib","/morelibs"])
L5) find([ "N.dll" ], ["/lib/../bin","/lib","/morelibs"])
X1) find(["X", "X.exe","X.com"], ["/bin","/moreexes"])
X2) find(["X", "X.exe" ], ["/bin","/moreexes"])
X3) find(["X", "X.com" ], ["/bin","/moreexes"])
Where the find method does not need to actually know whether it searches
for a dll or an exe, but dlopen() and exec() instead define the file
names to search for. This is what the patch draft does in dlopen.

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner
*) The directory of the current main executable should be searched
after LD_LIBRARY_PATH and before /usr/bin:/usr/lib.
And PATH should be searched before /usr/bin:/usr/lib as well.

Checking the executable path and $PATH are Windows concepts. dlopen
doesn't do that on POSIX systems and we're not doing that either.

Agreed, but POSIX also does have the concept of embedded RUNPATH,
which is completely missing in Cygwin as far as I can see.

RPATH and RUNPATH are ELF dynamic loader features, not supported by
PE/COFF.

In any case, to me it does feel quite important to have the (almost) same
dll search algorithm with dlopen() as with CreateProcess().

Last but not least I'm not yet convinced if it's *really* a good idea to
prepend the executable path to the DLL search path unconditionally. Be
it as it is in terms of DT_RUNPATH, why is the application dir a good
choice at all, unless we're talking Windows semantics? Which we don't.
Also, if loading from the applications dir from dlopen is important for
you, you can emulate it by adding the application dir to LD_LIBRAYR_PATH.

As long as there is lack of a Cygwin specific dll loader to find the
dlls to load during process startup, we're bound to Windows semantics.
For dlopen, it is more important to find the same dll file as would be
found when the exe was linked against that dll file, rather than using
the Linux-known algorithm and environment variables - and differ from
process startup: Both really should result in the same algorithm here,
even if that means some difference compared to Linux.
As far as I understand, lack of DT_RUNPATH (besides /etc/ld.so.conf)
support during process start was the main reason for the dlls to install
into /lib/../bin instead of /lib at all, to be found at process start
Why should that be different for dlopen?

Post by Corinna Vinschen
I checked for the usage of DT_RUNPATH/DT_RPATH on Fedora 23 and only a
limited number of packages use it (texlive, samba, python, man-db,
swipl, and a few more). Some of them, like texlive, even use it wrongly,
with RPATH pointing to a non-existing build dir. There are also a few
stray "/usr/lib64" settings, but all in all it's not used to point to
the dir the application is installed to, but rather to some package specific
subdir, e.g. /usr/lib64/samba, /usr/lib64/swipl-7.2.3/lib/x86_64-linux,
etc.

On Linux, the binaries installed in /usr usually rely on the Linux
loader to be configured via /etc/ld.so.conf to find their runtime
libs in /usr/lib.
Please remember: This whole thing is not a problem with packages
installed to /usr, but with packages installed to /somewhere/else
that provide runtime libraries that are also available in /usr.
Using LD_LIBRARY_PATH pointing to /somewhere/else/lib may break the
binaries found in /usr/bin - and agreed, searching PATH doesn't make
it better, as PATH is the "LD_LIBRARY_PATH" for Windows.

Post by Corinna Vinschen
IMHO this means just adding the applications bin dir is most of the time
an unused or even wrong workaround.

Although GetModuleHandle may reduce that pressure for dlopen - as long as
the applications bin dir is searched at process start, it really should
be searched by dlopen too, even if for /usr/bin/* this might indeed become
redundant, as we always add /usr/bin in dlopen - which really mimics
the /etc/ld.so.conf content actually, although that one is unavailable
to process startup.
Thanks!
/haubi/

[Corinna Vinschen]

This post might be inappropriate. Click to display it.

[Corinna Vinschen]

Hi Michael,

Post by Corinna Vinschen

Post by Michael Haubenwallner
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.

After a night's sleep something else occured to me. The old, existing
code didn't have to use {c}malloc/{c}free at all (ignoring called
functions). By using the pathfinder method in its current state
you now allocate/free multiple blocks, one for each basename and one for
each search dir. And at the end of the function you free the space
again.

Malloc/free is a costly operation which also requires locking and using
pathfinder as is adds that multiple times for each call of dlopen.
Every time we can get rid of malloc/free is a win. So, here's a
question: What if you only use a single tmp_pathbuf for the basenames
and a single tmp_pathbuf of 64K for the paths. That should be
sufficient for this functionality and the necessary buffers are already
allocated. Alternatively you could use alloca for the basenames, they
are light on space anyway.


Thanks,
Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen
Hi Michael,

Post by Michael Haubenwallner
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

Post by Corinna Vinschen

Post by Michael Haubenwallner
+ /* 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.

Post by Michael Haubenwallner
@@ -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.

Post by Corinna Vinschen

Post by Michael Haubenwallner
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.

Post by Corinna Vinschen

Post by Michael Haubenwallner
+ /* 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.

Post by Corinna Vinschen

Post by Michael Haubenwallner
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?

Post by Corinna Vinschen
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/

[Corinna Vinschen]

Hi Michael,

Post by Michael Haubenwallner
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?

Well, yes. tmp_pathbuf is meant to be initialized on function entry
(more or less, depends). It's supposed to exist only once per frame.
When the frame goes out of scope, the tmp_pathbuf usage counter is
restored to the values of the parent frame.

Post by Michael Haubenwallner
+ ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
+ when another instance on a newer stack frame has provided memory. */

I don't understand this comment, though. tmp_pathbuf can be used
multiple times in the same thread stackm see other Cygwin functions.
What you can't do is to call a function, instantiate tmp_pathbuf,
allocate memory in the called function, and then use it in the caller.
Well, you *can* do that, but if you do this more than once, the same
memory region is reused.
That's the whole idea of tmp_pathbuf.
Temporary per-thread memory for the current frame and it's child frames
is allocated. If a deeper frame using tmp_pathbuf goes out of scope,
the memory is not free'd, but recycled next time temporary memory is
needed. The buffers are either 32K or 64K to matches the maximum long
path length. They are now used for any purpose where larger temporary
per-thread memory is needed, but providing temporary long path buffers
without killing the stack was their original purposes.


Corinna

[Corinna Vinschen]

Post by Corinna Vinschen
Hi Michael,

Post by Michael Haubenwallner
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?

Well, yes. tmp_pathbuf is meant to be initialized on function entry
(more or less, depends). It's supposed to exist only once per frame.
When the frame goes out of scope, the tmp_pathbuf usage counter is
restored to the values of the parent frame.

Post by Michael Haubenwallner
+ ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
+ when another instance on a newer stack frame has provided memory. */

I don't understand this comment, though. tmp_pathbuf can be used
multiple times in the same thread stackm see other Cygwin functions.
What you can't do is to call a function, instantiate tmp_pathbuf,
allocate memory in the called function, and then use it in the caller.
Well, you *can* do that, but if you do this more than once, the same
memory region is reused.
That's the whole idea of tmp_pathbuf.
Temporary per-thread memory for the current frame and it's child frames
is allocated. If a deeper frame using tmp_pathbuf goes out of scope,
the memory is not free'd, but recycled next time temporary memory is
needed. The buffers are either 32K or 64K to matches the maximum long
path length. They are now used for any purpose where larger temporary
per-thread memory is needed, but providing temporary long path buffers
without killing the stack was their original purposes.

So I also don't quite understand splitting off tmp_pathbuf_allocator.

Why didn't you just include a tmp_pathbuf member into class pathfinder,
kind of like this:

/* pathfinder.h */
class pathfinder
{
tmp_pathbuf tp;
[...]
};

/* dlfcn.cc */
get_full_path_of_dll()
{
pathfinder finder (); /* Has a tmp_pathbuf member */

[...]
finder.add_basename (basename);
[...]
finder.add_searchdir (dir, ...);


return true; /* finder goes out of scope, so
its tmp_pathbuf goes out of scope
so tmp_pathbuf::~tmp_pathbuf is
called and all is well. */
}

?


Corinna

[Corinna Vinschen]

Post by Corinna Vinschen
Why didn't you just include a tmp_pathbuf member into class pathfinder,
/* pathfinder.h */
class pathfinder
{
tmp_pathbuf tp;
[...]
};
/* dlfcn.cc */
get_full_path_of_dll()
{
pathfinder finder (); /* Has a tmp_pathbuf member */
[...]
finder.add_basename (basename);
[...]
finder.add_searchdir (dir, ...);
return true; /* finder goes out of scope, so
its tmp_pathbuf goes out of scope
so tmp_pathbuf::~tmp_pathbuf is
called and all is well. */
}
?

Apart from these minor bits and pieces, I really like this new
pathfinder class, btw.


Corinna

[Michael Haubenwallner]

Post by Corinna Vinschen
Apart from these minor bits and pieces, I really like this new
pathfinder class, btw.

Thank you! :)

While at it: Combining dlopen thoughts with the "forkables" background
around replacing dlls-in-use leads me to this thought for redundant
calls of dlopen:

As we've already agreed that GetModuleHandleEx might make sense,
I'm thinking whether pathfinder could be useful even for calling
GetModuleHandleEx before actually testing for the existing file.

First, we need to agree on which search order is correct for a
specific (redundant) dlopen usage. IMO, this should be for:

dlopen ("libz.so"):
1) GetModuleHandleEx ("libz.so")
2) GetModuleHandleEx ("cygz.dll")
3) GetModuleHandleEx ("libz.dll")
4) path_conv.exists-based only, as with current pathfinder

dlopen ("/usr/lib/libz.so"):
1) GetModuleHandleEx ("\winpath\of\usr\bin\libz.so")
2) GetModuleHandleEx ("\winpath\of\usr\bin\cygz.dll")
3) GetModuleHandleEx ("\winpath\of\usr\bin\libz.dll")

4) GetModuleHandleEx ("\winpath\of\usr\lib\libz.so")
5) GetModuleHandleEx ("\winpath\of\usr\lib\cygz.dll")
6) GetModuleHandleEx ("\winpath\of\usr\lib\libz.dll")

7) path_conv.exists ("\winpath\of\usr\bin\libz.so")
8) path_conv.exists ("\winpath\of\usr\bin\cygz.dll")
9) path_conv.exists ("\winpath\of\usr\bin\libz.dll")

10) path_conv.exists ("\winpath\of\usr\lib\libz.so")
11) path_conv.exists ("\winpath\of\usr\lib\cygz.dll")
12) path_conv.exists ("\winpath\of\usr\lib\libz.dll")

Thing is that with the latter (predefined path), in a first iteration
we need the same filenames to test for already-loaded as in a second
iteration where we test for real files.

Attaching a patch draft for a pathfinder criterion interface...

Thoughts?

/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner

Post by Corinna Vinschen
Apart from these minor bits and pieces, I really like this new
pathfinder class, btw.

Thank you! :)
While at it: Combining dlopen thoughts with the "forkables" background
around replacing dlls-in-use leads me to this thought for redundant
As we've already agreed that GetModuleHandleEx might make sense,

Erm... where exactly did we do that? I recall I mentioned using
LoadLibraryEx with LOAD_LIBRARY_SEARCH_USER_DIRS and the AddDllDirectory
functions, but I don't think we talked about GetModuleHandleEx.

The question is if it really makes sense to add calls to
GetModuleHandleEx. Apart from slowing down the dlopen call, it seems
unnecessary. Assuming you call LoadLibrary with the same paths and same
extensions in the same order, wouldn't a second call to dlopen have the
exact same result (loading the same file)?


Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
Apart from these minor bits and pieces, I really like this new
pathfinder class, btw.

Thank you! :)
While at it: Combining dlopen thoughts with the "forkables" background
around replacing dlls-in-use leads me to this thought for redundant
As we've already agreed that GetModuleHandleEx might make sense,

Erm... where exactly did we do that? I recall I mentioned using
LoadLibraryEx with LOAD_LIBRARY_SEARCH_USER_DIRS and the AddDllDirectory
functions, but I don't think we talked about GetModuleHandleEx.

Indeed not for dlopen calls including a path - but for without a path:
https://sourceware.org/ml/cygwin-developers/2016-06/msg00003.html

Post by Corinna Vinschen
The question is if it really makes sense to add calls to
GetModuleHandleEx. Apart from slowing down the dlopen call, it seems
unnecessary. Assuming you call LoadLibrary with the same paths and same
extensions in the same order, wouldn't a second call to dlopen have the
exact same result (loading the same file)?

Sure - as long as an already loaded dll is not replaced by an updated one.
Before I'll gonna try: Do you know by chance what LoadLibrary does here?

Artifical corner case LoadLibrary will never handle:
* 1st dlopen("libN.so") loads "libN.dll"
* same program installs newer package N, now providing "cygN.dll"
* 2nd dlopen("libN.so") loads "cygX.dll": expected is old "libN.dll" handle
This is true for both dlopen("libN.so") and dlopen("/path/lib/libN.so").

However, for perfomance reasons I can imagine some shared flag for both
dlopen and fork (others?): "support updating running binaries (NTFS only)",
to avoid the extra GetModuleHandleEx calls in dlopen by default.

/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner
Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
Apart from these minor bits and pieces, I really like this new
pathfinder class, btw.

Thank you! :)
While at it: Combining dlopen thoughts with the "forkables" background
around replacing dlls-in-use leads me to this thought for redundant
As we've already agreed that GetModuleHandleEx might make sense,

Erm... where exactly did we do that? I recall I mentioned using
LoadLibraryEx with LOAD_LIBRARY_SEARCH_USER_DIRS and the AddDllDirectory
functions, but I don't think we talked about GetModuleHandleEx.

https://sourceware.org/ml/cygwin-developers/2016-06/msg00003.html

Post by Corinna Vinschen
The question is if it really makes sense to add calls to
GetModuleHandleEx. Apart from slowing down the dlopen call, it seems
unnecessary. Assuming you call LoadLibrary with the same paths and same
extensions in the same order, wouldn't a second call to dlopen have the
exact same result (loading the same file)?

Sure - as long as an already loaded dll is not replaced by an updated one.
Before I'll gonna try: Do you know by chance what LoadLibrary does here?

No, I don't know exactly, but in theory it should try to match
the incoming filename against the filename stored in the loader
list attached to the PEB. This would even work if the DLL has been
renamed in the meantime.


Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Corinna Vinschen

Post by Michael Haubenwallner
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?

Well, yes. tmp_pathbuf is meant to be initialized on function entry
(more or less, depends). It's supposed to exist only once per frame.
When the frame goes out of scope, the tmp_pathbuf usage counter is
restored to the values of the parent frame.

Post by Michael Haubenwallner
+ ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
+ when another instance on a newer stack frame has provided memory. */

I don't understand this comment, though. tmp_pathbuf can be used
multiple times in the same thread stackm see other Cygwin functions.
What you can't do is to call a function, instantiate tmp_pathbuf,
allocate memory in the called function, and then use it in the caller.
Well, you *can* do that, but if you do this more than once, the same
memory region is reused.
That's the whole idea of tmp_pathbuf.
Temporary per-thread memory for the current frame and it's child frames
is allocated. If a deeper frame using tmp_pathbuf goes out of scope,
the memory is not free'd, but recycled next time temporary memory is
needed. The buffers are either 32K or 64K to matches the maximum long
path length. They are now used for any purpose where larger temporary
per-thread memory is needed, but providing temporary long path buffers
without killing the stack was their original purposes.

So I also don't quite understand splitting off tmp_pathbuf_allocator.

Sorry, 've expected to have provided enough reasoning here:
https://cygwin.com/ml/cygwin-developers/2016-08/msg00016.html

Actually it is vstrlist doing the alloc+free, and I do expect vstrlist
to be useful even in scenarios where it does not work to ignore it's
free calls, but requires the allocator to fully maintain the alloc'ed
regions to allow for free'd regions to become reused.
And as I've stumbled over 3 different memory providers already (malloc,
cmalloc, tmp_pathbuf), I don't think vstrlist should know anything
about tmp_pathbuf at all - otherwise it should be named tmp_vstrlist.

Same stands for pathfinder, which needs the allocator interface reference
only for construction of the embedded vstrlist that holds the searchdirs.

Post by Corinna Vinschen
Why didn't you just include a tmp_pathbuf member into class pathfinder,
/* pathfinder.h */
class pathfinder
{
tmp_pathbuf tp;
[...]
};
/* dlfcn.cc */
get_full_path_of_dll()
{
pathfinder finder (); /* Has a tmp_pathbuf member */
[...]
finder.add_basename (basename);
[...]
finder.add_searchdir (dir, ...);
return true; /* finder goes out of scope, so
its tmp_pathbuf goes out of scope
so tmp_pathbuf::~tmp_pathbuf is
called and all is well. */
}

Well, as one stackframe better uses one _single_ instance of tmp_pathbuf,
I prefer to be explicit about this and have pathfinder+vstrlist take a
tmp_pathbuf _reference_:
A member instance would be hidden to some degree, and thus will ask for
trouble in the long run, as it would not be obvious to developers in the
future that this stackframe already _does_ have that single instance and
one should not add (another) one.

In hope to provide graspable thoughts,
/haubi/

[Michael Haubenwallner]

Post by Corinna Vinschen
Hi Michael,

Post by Michael Haubenwallner
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?

Well, yes. tmp_pathbuf is meant to be initialized on function entry
(more or less, depends). It's supposed to exist only once per frame.
When the frame goes out of scope, the tmp_pathbuf usage counter is
restored to the values of the parent frame.

Post by Michael Haubenwallner
+ ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
+ when another instance on a newer stack frame has provided memory. */

I don't understand this comment, though.

Problem is that while the second tmp_pathbuf is constructed,
the first tmp_pathbuf must not be asked for another buffer,
because destructing the second tmp_pathbuf will reset the
tls.counter to what it was before constructing the second,
causing the first tmp_pathbuf to return buffers *again* which
it may have returned already while the second one was alive.

I've had something like this scope flow breaking, where pathfinder
used tmp_pathbuf tpF as its own instance, while the local stack
used tmp_pathbuf tpL:

{
pathfinder finder (w_buf_old=0); // tls.w_cnt is 0
finder.add_some_dirs(...); // tls.w_cnt is 1 now (by tpF)
{
tmp_pathbuf tpL (w_buf_old=1); // tls.w_cnt is 1 still
finder.add_some_dirs(...); // tls.w_cnt is 2 now (by tpF)
PWCHAR exewname = tpL.w_get (); // tls.w_cnt is 3 now (by tpL)
GetModuleFileNameW ( exewname );
finder.add_dir (from exewname); // tls.w_cnt is 4 now (by tpF)
} // destruct tpL (w_buf_old==1) // tls.w_cnt is 1 now (restored by ~tpL)
finder.add_some_dirs(...); // tls.w_cnt is 2 now (tpF already returned that above)
// here the memory provided by tpF since first time tls.w_cnt=2
// is overwritten due to tpF returning the same buffers again!
}

/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner

Post by Corinna Vinschen
Hi Michael,

Post by Michael Haubenwallner
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?

Well, yes. tmp_pathbuf is meant to be initialized on function entry
(more or less, depends). It's supposed to exist only once per frame.
When the frame goes out of scope, the tmp_pathbuf usage counter is
restored to the values of the parent frame.

Post by Michael Haubenwallner
+ ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
+ when another instance on a newer stack frame has provided memory. */

I don't understand this comment, though.

Problem is that while the second tmp_pathbuf is constructed,
the first tmp_pathbuf must not be asked for another buffer,
because destructing the second tmp_pathbuf will reset the
tls.counter to what it was before constructing the second,
causing the first tmp_pathbuf to return buffers *again* which
it may have returned already while the second one was alive.
I've had something like this scope flow breaking, where pathfinder
used tmp_pathbuf tpF as its own instance, while the local stack

Yeah, ok, that's not what tmp_pathbuf was designed for, it was strictly
a per-frame thingy.


Corinna

[Corinna Vinschen]

Post by Michael Haubenwallner

Post by Corinna Vinschen
(*) 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.

Btw., I just noticed something interesting, independently of your patch.
Consider the file /usr/bin/cygz.dll:

- dlopen (libz.so) success

- dlopen (/usr/bin/libz.so) success

- dlopen (/usr/lib/libz.so) fails

That's pretty clear when looking through the code, but... wouldn't
it make sense to allow that? If a path is given, and the path points
to /usr/lib, search the file in /usr/bin as well?


Thanks,
Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
(*) 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.

Btw., I just noticed something interesting, independently of your patch.
- dlopen (libz.so) success
- dlopen (/usr/bin/libz.so) success
- dlopen (/usr/lib/libz.so) fails
That's pretty clear when looking through the code, but... wouldn't
it make sense to allow that? If a path is given, and the path points
to /usr/lib, search the file in /usr/bin as well?

Easy enough - but this should apply to any prefix IMO: While the
application specific prefix often isn't /usr - but something like
/usr/local or /opt/application, application specific libs may be
built & installed with libtool or something similar as well - at
least some tool that knows about installing the real dll into
<app-prefix>/bin (because of the missing Cygwin loader).

But agreed, it makes sense doing /lib->/bin for the explicit path and
the /usr/lib default only and not for the environment-provided paths.

/haubi/

[Corinna Vinschen]

Post by Michael Haubenwallner
Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
(*) 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.

Btw., I just noticed something interesting, independently of your patch.
- dlopen (libz.so) success
- dlopen (/usr/bin/libz.so) success
- dlopen (/usr/lib/libz.so) fails
That's pretty clear when looking through the code, but... wouldn't
it make sense to allow that? If a path is given, and the path points
to /usr/lib, search the file in /usr/bin as well?

Easy enough - but this should apply to any prefix IMO: While the
application specific prefix often isn't /usr - but something like
/usr/local or /opt/application, application specific libs may be
built & installed with libtool or something similar as well - at
least some tool that knows about installing the real dll into
<app-prefix>/bin (because of the missing Cygwin loader).

You have a point there.

Post by Michael Haubenwallner
But agreed, it makes sense doing /lib->/bin for the explicit path and
the /usr/lib default only and not for the environment-provided paths.

It feels certainly more safe to restrict this to the system path for
now. But... yeah, you have a point.

Not well thought out, just an idea kicking around:

Apart from the obvious system path handling, what if other lib->bin
transitions only take place if the calling application is installed
in that very bin dir...?


Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
(*) 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.

Btw., I just noticed something interesting, independently of your patch.
- dlopen (libz.so) success

This one succeeds because of /usr/bin being the fallback path, but ...

Post by Corinna Vinschen

Post by Michael Haubenwallner

Post by Corinna Vinschen
- dlopen (/usr/bin/libz.so) success
- dlopen (/usr/lib/libz.so) fails
That's pretty clear when looking through the code, but... wouldn't
it make sense to allow that? If a path is given, and the path points
to /usr/lib, search the file in /usr/bin as well?

Easy enough - but this should apply to any prefix IMO: While the
application specific prefix often isn't /usr - but something like
/usr/local or /opt/application, application specific libs may be
built & installed with libtool or something similar as well - at
least some tool that knows about installing the real dll into
<app-prefix>/bin (because of the missing Cygwin loader).

You have a point there.

Thanks!

... I forgot about dlopen("libAPP.so") (without path): This I expect
to find <app-prefix>/bin/cygAPP.dll - which is the application dir.

Post by Corinna Vinschen

Post by Michael Haubenwallner
But agreed, it makes sense doing /lib->/bin for the explicit path and
the /usr/lib default only and not for the environment-provided paths.

It feels certainly more safe to restrict this to the system path for
now. But... yeah, you have a point.
Apart from the obvious system path handling, what if other lib->bin
transitions only take place if the calling application is installed
in that very bin dir...?

Interesting idea - might work indeed! Even for prefix=/usr, to
have consistent behaviour across different application prefixes.

For safety regarding the application dir: If one can write to the
application dir, couldn't one put a malicious kernel32.dll there
as well, and/or an empty application.exe.local for dll redirection?

/haubi/

[Corinna Vinschen]

Hi Michael,

Post by Michael Haubenwallner

Post by Corinna Vinschen
Apart from the obvious system path handling, what if other lib->bin
transitions only take place if the calling application is installed
in that very bin dir...?

Interesting idea - might work indeed! Even for prefix=/usr, to
have consistent behaviour across different application prefixes.

Actually, no. This test is not ok for the system DLL path, because
system DLLs are expected to exist for all applications, even those
not installed in a a system path itself.

Post by Michael Haubenwallner
For safety regarding the application dir: If one can write to the
application dir, couldn't one put a malicious kernel32.dll there
as well, and/or an empty application.exe.local for dll redirection?

I'm not overly fluent with the .local stuff, but the kernel32.dll thingy
should work as desired since kernel32.dll is one of the KnownDLLs.


Corinna

[Corinna Vinschen]

Hi Michael,

Post by Michael Haubenwallner
For dlopen, it is more important to find the same dll file as would be
found when the exe was linked against that dll file, rather than using
the Linux-known algorithm and environment variables - and differ from
process startup: Both really should result in the same algorithm here,
even if that means some difference compared to Linux.

IMHO there is no good reason to have a DLL in a 3rd party subdir which
is available as system DLL. DT_RPATH/DT_RUNPATH semantics are not
created to overload system DLLs, rather to add a safe search path. Why
would you want to install a system DLL under the same name and thus the
same version in a non- system dir and expect the application to load
that? Ultimately this is bound to becoming outdated, fail if the
dependencies of the system DLL change, etc.

Even *if* we add the application dir adding it to dlopen should be
configurable, not statically, invisible under the hood.


Corinna

[Michael Haubenwallner]

Hi Corinna,

Post by Corinna Vinschen

Post by Michael Haubenwallner
For dlopen, it is more important to find the same dll file as would be
found when the exe was linked against that dll file, rather than using
the Linux-known algorithm and environment variables - and differ from
process startup: Both really should result in the same algorithm here,
even if that means some difference compared to Linux.

IMHO there is no good reason to have a DLL in a 3rd party subdir which
is available as system DLL. DT_RPATH/DT_RUNPATH semantics are not
created to overload system DLLs, rather to add a safe search path. Why
would you want to install a system DLL under the same name and thus the
same version in a non- system dir and expect the application to load
that? Ultimately this is bound to becoming outdated, fail if the
dependencies of the system DLL change, etc.

This highly depends on the definition of which DLLs are part of the
"system", and which DLLs are part of the "application".

Just installing a DLL into /usr/bin does not mean it is a "system DLL"
which an application safely can rely on.

Instead, an application expected to be "stable" has to ship dependencies
along the application itself, as it cannot expect the "system" to provide
the identical (versions of) dependencies the application was regression
tested against.

Agreed, this asks for outdated dependencies. But this is the reason why
the update mechanism of the application also is able to update its own
dependencies - within the application directory.

This allows for always running a regression tested application, which
is expected to not break just because some random DLL is installed or
updated within /usr/bin.

For the definition of "system": As the application is required to run
on lots of different operating systems and architectures, a sensible
definition of "system" that allows for most portability with least
porting effort (code duplication) is to stick with "libc", along the
lines of "POSIX" as close as possible.

Post by Corinna Vinschen
Even *if* we add the application dir adding it to dlopen should be
configurable, not statically, invisible under the hood.

Independent of application stability and portability: Is there a reason
for dlopen to potentially find different DLLs than process startup does?

Thanks!
/haubi/