319658 - Problems loading shared libs on Windows when paths start with "." or use forward slashes

Status: NEW

(NSPR :: NSPR, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

We received a report from an application developer that NSS was loading 
nssckbi.dll in the process during NSS initialization, even though nssckbi.dll 
was not specified in the secmod.db file, and was not present in the directory 
where the DB files live. nssckbi.dll was present in a directory in the PATH.  
It was being loaded even though the application did not wish for it to be 
loaded.  The problem is seen only on Windows, not on Unix.  The directory 
name being passed to NSS_Init is "." .

Neil Williams and I figured out why, and discovered a number of other problems
related to the causes.  These problems may be perceived as Windows OS problems,
and/or NSPR problems, and/or NSS problems.  I recommend that all these things 
be fixed in NSPR, but am willing to be persuaded that they be fixed in NSS 
instead.  Perhaps multiple bugs need to be filed.  
Here are the findings and issues:

1. NSS and/or NSPR need to convert all forward slashes to back slashes in 
shared library (path) names before passing them to Windows' LoadLibrary 
function.  LoadLibrary decides whether the string is a path name or a 
simple file name, because the rules for the two cases are quite different.
It searches the string for colons and for backslashes, but not for forward 
slashes.  So, if the string is a path name that uses only forward slashes 
as path separators and has no drive letter prefix (e.g. C:) LoadLibrary 
will conclude that it is a simple file name, not a path name.  For example:
/windows/system32/foo.dll will be treated as a simple file name, not as a 
path name.  This will cause unexpected failures.

I recommend that PR_LoadLibrary(Ex) should make a copy of the input name 
and replace all forward slashes with backslashes, and then call LoadLibrary 
with that modified string.

2. PR_LoadLibrary(Ex) has an internal list of loaded "modules" and attempts 
to avoid duplicate loads by comparing the base name of the shared lib to 
be loaded with each of the base names of the loaded modules.  Unfortunately,
its algorithm for finding the base name only checks for back slahes. 
So, if the input string uses only forward slashes as path separators,
it will never be found to match an already-loaded module.

My recommendation (above) that PR_LoadLibrary(Ex) should convert forward 
slashes to back slashes would also solve this problem.

3. Windows' LoadLibrary function has a peculiar problem with library names
that begin with "./" or ".\" followed by a simple name.  It ALWAYS thinks
those names are simple file names, not path names.  For example, the strings
"./nsskcbi.dll" and "./nssckbi.dll" will be treated as simple file names,
not as path names.  LoadLibrary does not have this problem with names 
beginning with a drive letter, nor with names containing additional back 
slashes (e.g. ".\foo\bar.dll") nor with names that begin with "..\".  

Consequently, if NSS calls PR_LoadLibrary with "./nssckbi.dll", and 
nssckbi.dll does NOT live in the current workding directory (cwd), but
DOES live in any of the directories in the PATH, it will be loaded from
the PATH.  

I recommend this solution:  After converting all forward slashes to back slashes, PR_LoadLibrary should determine if the name begins with ".\"
and contains no other path separators (no other back slashes, and does 
not begin with a drive letter such as C:).  If the name does fit that 
description, then NSPR should replace the leading "." with the absolute 
current working directory name (using back slashes).  That will ensure
that the name is treated as a path name and not as a simple file name.

4. NSS uses forward slash as a hard-coded path separator character, 
regardless of platform, in many places.  This is generally not a problem
for opening files, but is a problem with (PR_)LoadLibrary.  I recommend
that it be fixed in NSPR, rather than in NSS.

Comment 1

[Robert Relyea]

There is an NSS initialization option which will force NSS not to automatically load builtins: NSS_INIT_NOROOTINIT

The builtins are only loaded automatically if you call the NSS_Initialize() function without passing NSS_INIT_NOROOTINIT (it isn't initialized automatically if you call NSS_Init, NSS_InitReadWrite, or NSS_NoDB_Init).

The simple solution of the application writer is to just pass in NSS_INIT_NOROOTINIT (automatic builtins loading was a hack to preserve binary compatibility for server products).

I'm not closing this bug, because NSS_INIT_NOROOTINIT should solve the developer's problem, but the general problem of inconsistent paths on Windows still exists.

bob

bob

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Bob, it is not the case that this developer *never* wants to use nssckbi.

This developer wants NSS to behave consistently, the same way on Windows
as on Unix, with respect to whether or not NSS loads nssckbi when it is 
not in secmod.db and is not in the DB directory, regardless of the choice
of the DB directory name.  

Today, Windows and Unix behave very much alike, except for certain specific 
cases, one of which is when the DB directory is ".".  

We have stated that NSS_Init loads nssckbi if any of the following is true:
a) the secmod.db file tells us to load it (and it is loadable with the
   (path)name given in secmod.db), or
b) it is found in the DB directory,
and not otherwise.  The developer simply wants NSS to honor that statement
on both Windows and Unix.

Comment 3

[Julien Pierre]

Neil,

What happens if one passes .\.\sharedlib.dll or ././sharedlib.dll to LoadLibrary() ? Does it still load it from the PATH or not ? You mentioned that multiple slashes or backslashes solve the problem, so I'm wondering if this would trick it into not thinking it's a simple filename.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Sun would really like to see this fixed for JES 5 before end of 2006.
I don't know what target NSPR milestone to set for that.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch changes the behavior on WIN32 of two NSPR functions: 
pr_LoadLibraryByPathname and
PR_LoadStaticLibrary

These functions now:
a) convert all forward slashes to back slashes in path names, and
b) change any pathnames that start with ".\", replacing the leading "." 
with the current working directory.  

These two changes are intended to have the effect that programs using 
NSPR on Windows will now see behavior consistent with behavior on UNIX,
with respect to determining if the library file name is a simple name 
or a path name, and searching the (library) path ONLY if it is a simple 
file name, not a pathname.  

Here is the test case I used to test this:
  
With both the NSS/NSPR "bin" and "lib" directories in the PATH, 
and with nssckbi.dll in the "lib" directory, 
cd to an empty directory and run these commands:
   certutil -d . -L -X
   modutil -dbdir . -list

With the currently released versions of NSPR and NSS, these commands 
will show that "./nssckbi.dll" was added to the list of PKCS#11 modules,
and that module was successfully loaded, EVEN THOUGH no file by the name
of ./nssckbi.dll exists!  This is the erroneous behavior seen only on
Windows.

Then, using NSPR with this patch, and a fresh empty directory, repeat 
the test.  The modutil output will show that nssckbi.dll is NOT in the
list of PKCS#11 modules and is not loaded.  This is the correct behavior,
matching the behavior seen on Unix/Linux systems.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Well, I found one problem with this patch.  In PR_LoadStaticLibrary, 
if the attempt to allocate memory for "lm" fails, leaving lm NULL,
the code leaks dupName.  I'll fix that, but I'd like to get other 
review comments first.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 251251 [details] [diff] [review]
patch v1

I just spotted another issue with this patch.  

> #ifdef WIN32
>     if (flags & PR_LD_PATHW) {
>         /* cast back what's cast to |char *| for the argument passing. */
>-        wname = (LPWSTR) name;
>+        wname = wname_malloc = _wcsdup((LPWSTR)name);

Here I need to insert:

>+        if (wname == NULL) {
>+            oserr = _MD_ERRNO();
>+            goto unlock;

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: trunk patch, v2

This patch fixes the problems I caught after submitting the previous one.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 253440 [details] [diff] [review]
trunk patch, v2

Please review this patch for inclusion in the mid-February release.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 253440 [details] [diff] [review]
trunk patch, v2

Seeking two reviews, best two out of three. ;)

Comment 11

[Alexei Volkov]

Comment on attachment 253440 [details] [diff] [review]
trunk patch, v2


Patch looks good. 

The two things I've notice: 

  dupName will be leaked on all platforms except windows if "result", returned by pr_UnlockedFindLibrary(dupName), is equal NULL

>+    dupName = strdup(name);
>+    if (!dupName) {
>+	oserr = _MD_ERRNO();
>+    	goto unlock;
>+    }

  and that the following error should also be set for all platforms, not only for windows.

>>     PR_ExitMonitor(pr_linker_lock);
>+#ifdef WIN32
>+    if (result == NULL) {
>+        PR_SetError(PR_LOAD_LIBRARY_ERROR, oserr);
>+        DLLErrorInternal(oserr);  /* sets error text */
>+    }

Comment 12

[Neil Williams]

Comment on attachment 253440 [details] [diff] [review]
trunk patch, v2

In PR_LoadStaticLibrary() you added code to copy name to wname, perform the directory separator changes on it and then do this:

dupName = strdup(name);

after which dupname is used in place of name. What happened to wname? Did I miss an assignement statement?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 253440 [details] [diff] [review]
trunk patch, v2

Good catch, Neil!  Thanks.

I'm thinking maybe I attached the wrong patch file to this bug. 
In any case, I will fix it.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: trunk patch, v3

This should be better.

The patch fixes two NSPR functions: pr_LoadLibraryByPathname and 
PR_LoadStaticLibrary.  I was only testing pr_LoadLibraryByPathname
which is why I didn't catch this in testing.

Comment 15

[Neil Williams]

Comment on attachment 256553 [details] [diff] [review]
trunk patch, v3

That's better.

Comment 16

[Wan-Teh Chang]

Attachment: Wan-Teh's version of "trunk patch, v3"

Nelson, this patch is the best way to describe my suggested
changes.

I also have a general comment.  PR_LoadStaticLibrary is most likely
dead code.  It don't know exactly how it works, and there is no
useful test for it.  So I suggest that we not bother updating this
function.

Here are brief comments on the more subtle changes.

1. Declare wlen in the function scope so that we can save its
value for later use.
2. Carefully match the allocation function with the free function.
3. Ensure the wname buffer can hold at least MAX_PATH wide chars.
This is needed when we copy wcwd back to wname.
4. With your change, we should always call LoadLibraryW(wname).

Comment 17

[Wan-Teh Chang]

Comment on attachment 256553 [details] [diff] [review]
trunk patch, v3

Also note that PR_LoadStaticLibrary doesn't call LoadLibrary, so it
doesn't need to work around the bugs of LoadLibrary.  We only need
to convert forward slashes to backslashes so that pr_UnlockedFindLibrary
will correctly find the file name portion of the path name.  The
conversion of '.' to cwd is not necessary.

Comment 18

[Wan-Teh Chang]

Comment on attachment 256553 [details] [diff] [review]
trunk patch, v3

I found that _wgetcwd only works on the NT series of Windows.
(See the Requirements section in
http://msdn2.microsoft.com/en-us/library/sf98bd4y(VS.80).aspx)
We should at least make sure we handle the failure of _wgetcwd
gracefully on Windows 98.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256848 [details] [diff] [review]
Wan-Teh's version of "trunk patch, v3"

Wan-Teh's helpful comment 17 and comment 18 show that this bug needs
a lot more work.  So, I'm giving it r- on that basis.  
Given that _wgetcwd doesn't work on all supported windows platforms,
I think we have to stop using it entirely, and perhaps implement 
our own.  I doubt this bug fix will make it into NSPR 4.6.7 with the 
time remaining.

Wan-Teh you made one change to the patch that I question. 
You changed my patch's < to <= at the point shown below. 

>+	PRUnichar wcwd_stack[MAX_PATH];
>+    	PRUnichar *wcwd = _wgetcwd(wcwd_stack, MAX_PATH);
>+	if (wcwd) {
>+	    int cwdlen = wcslen(wcwd);
>+	    int offset = (wcwd[cwdlen - 1] == PR_DIRECTORY_SEPARATOR ? 2 : 1);
>+	    if (wlen + cwdlen <= MAX_PATH) {
                              ^^
>+		wcscat(wcwd, wname + offset);
>+	    	wcscpy(wname, wcwd);
>+	    }
>+	}

IINM, if wlen+cwdlen == MAX_PATH, then the wcscat call will attempt to 
write an unicode NUL character at the two-bytes immediately following 
the array wcwd_stack in the stack, a stack buffer overflow.  
We could increase the size of wcws_stack to MAX_PATH+1, or stay with 
a < test instead of a <= test.  

Do you agree? and if so, which do you recommend?

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Time has passed, and the concerns of comment 18 are no longer relevant because
we no longer support Windows 98.  So, it's time to look at this bug again.

Comment 21

[Christophe Ravel]

Could we have another look at this bug and verify that the current patch would work ?

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P1'.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.