480730 - PR_LoadLibrary for 64 bit MAC OS X ignores "./" and treats "./libname" same as "libname"

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[glen beasley]

Attachment: test program

64-bit Mac OS X NSPR uses the dlopen functions (declared in <dlfcn.h>) for dynamic library loading see bug 370766

On most OS's PR_LoadLibrary treats "./"  explicitly.

for Mac OS X:

   When path contains a slash (i.e. a full path or a partial path) dlopen() searches
     the following the following until it finds a compatible Mach-O file:
     $DYLD_LIBRARY_PATH (with leaf name from path ), current working directory (for par-
     tial paths), $DYLD_FALLBACK_LIBRARY_PATH (with leaf name from path ).

man dlopen
or
http://developer.apple.com/DOCUMENTATION/DeveloperTools/Reference/MachOReference/Reference/reference.html#//apple_ref/c/func/dlopen


meaning the Root Certs will be added provided ./libnssckbi.dylib is in the path not in the current directory

#0  pr_LoadLibraryByPathname (name=0x425450 "./libnssckbi.dylib", flags=1) at ../../../../pr/src/linking/prlink.c:1028
....
#9  0x001c8407 in NSS_Initialize (configdir=0x25900 ".", certPrefix=0x17ff2 "", keyPrefix=0x17ff2 "", secmodName=0x1c556 "secmod.db", flags=0) at nssinit.c:653
#10 0x0000aa78 in certutil_main (argc=4, argv=0xbffff784, initialize=1) at certutil.c:2361
#11 0x0000c5d8 in main (argc=4, argv=0xbffff784) at certutil.c:2981

Comment 1

[glen beasley]

Attachment: On Mac OS X 64 bit if a library name has a path check if path exists

Comment 2

[Wan-Teh Chang]

Comment on attachment 366365 [details] [diff] [review]
On Mac OS X 64 bit if a library name has a path check if path exists

r=wtc.  Some suggested changes before you check this in.

1. Use DARWIN instead of XP_MACOSX.

2. The comment should explain why you need to check the
path for existence.  Please add a concise summary of the
problem.

3. You have a typo in the comment: splash => slash.

4. You can rewrite the code like this:

    if (strchr(name, '/') == NULL ||
        PR_Access(name, PR_ACCESS_EXISTS) == PR_SUCCESS) {
        h = dlopen(name, dl_flags);
    }

5. I believe that it's also safe to call dlopen directly
if 'name' is an absolute pathname.  Could you test that?
If so, the above code becomes:

    if (name[0] == '/' || strchr(name, '/') == NULL ||
        PR_Access(name, PR_ACCESS_EXISTS) == PR_SUCCESS) {
        h = dlopen(name, dl_flags);
    }

Comment 3

[glen beasley]

Thanks wan-teh for the review. 
Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.100; previous revision: 3.99
done


I tested #5 with a private test program.

export DYLD_LIBRARY_PATH=/Users/b/tip/mozilla/dist/Darwin9.6.0_DBG.OBJ/lib
./dllLoad /tmp/libnssckbi.dylib
testing /tmp/libnssckbi.dylib
ERROR Library not found with provided path
ERROR path not valid but library loaded!

Open apple Problem ID: 6702829

Comment 4

[Wan-Teh Chang]

Glen, thanks for submitting a bug report to Apple.

Comment 5

[Kai Engert (:KaiE:)]

I think the effect of the old patch was:

• if there is no slash, call dlopen
• if there is slash, and that file doesn't exist, then DON'T call dlopen. If the file exists, then allow dlopen, even though dlopen might open a library that is located at a different location.

I don't understand why it was necessary to add the check for PR_Access.
I think this was not sufficiently explained.

Was this an attempt to prevent loading from a location that is different from the name?
That attempt couldn't have been successful. As explained above, the OS will always attempt to load from $DYLD_LIBRARY_PATH in the initial step, and will ignore the given path in the initial step.

It dlopen returns NULL if no library can be loaded - doesn't that have the same effect as detecting that the file does not exist?

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Kai Engert (:KaiE:) from comment #5)

I think the effect of the old patch was:

• if there is no slash, call dlopen
• if there is slash, and that file doesn't exist, then DON'T call dlopen. If the file exists, then allow dlopen, even though dlopen might open a library that is located at a different location.

I don't understand why it was necessary to add the check for PR_Access.

I agree. My interpretation is that the reporter's intent was to make PR_LoadLibrary() fail if ./libnssckbi.dylib did not exist in the current working directory. It doesn't make sense because if the library is present, DYLD_LIBRARY_PATH will still used before ./libnssckbi.dylib.

If you ignore DYLD_LIBRARY_PATH and only consider DYLD_FALLBACK_LIBRARY_PATH, the fix does allow the local ./libnssckbi.dylib to be used while preventing fallback to DYLD_FALLBACK_LIBRARY_PATH.

And the bug subject states dlopen treats "./libname" same as "libname". Paths without any slashes go through LD_LIBRARY_PATH so if ./libname was treated the same as libname, then LD_LIBRARY_PATH would be used which would be inconsistent with the man page. But there's no mention of that problem.

Comment 7

[Kai Engert (:KaiE:)]

We have a regression report in bug 1653310. Now I understand what was going on, and what was the original intention.

During startup time, the NSS library is initialized with a path to a database directory.

NSS has code that will automatically attempt to load the nssckbi shared library at startup.
It will prefix the library name with the directory name, which may be "./" or any other respective path.

Because of the macOS behavior to search libraries globally, this had the effect of loading the library from an unepected location, if there's no such library file in the database path.

This was undesired, because this library contains application defined data. So the application wants to strictly control which library is loaded.

Glen wanted to prevent that a global library is loaded. He decided to fix it by changing the lower level library loading code in NSPR.

I think this was an incorrect decision. The check for the file should be done at a higher level.

I'll suggest a fix in bug 1653310.

Comment 8

[Mike Hommey [:glandium]]

I think this was an incorrect decision. The check for the file should be done at a higher level.

I disagree.

Comment 9

[Kai Engert (:KaiE:)]

deleted

Comment 10

[Kai Engert (:KaiE:)]

sorry comment 9 was confusing. delete. let me try again.

Comment 11

[Mike Hommey [:glandium]]

But NSS is not calling dlopen, it's calling PR_LoadLibrary. And PR_LoadLibrary is not an alias to dlopen. That dlopen has caveats on some specific platforms is exactly why we have a cross-platform abstraction layer. So that not every single caller have to deal with these issues.

Comment 12

[Kai Engert (:KaiE:)]

I made an incorrect assumption initially.
I thought the intention was to guarantee that either the library at the given relative/absolute is loaded, or that no library is loaded.

But the code cannot guarantee that.
Despite a successful PR_Access check that the given relative/absolute path exists, dlopen might nevertheless load a completely different library.

This is why I had incorrectly concluded that the old fix was pointless.

Now I understand that the intention was different:
If the given path doesn't exist, NSS relies on PR_LoadLibrary to fail.
If the path exists, then NSS accepts that a different library might potentially get loaded.

I can agree that performing this check in PR_LoadLibrary for macOS can make sense, to partially simulate the behavior of failing library loading on other platforms.
It's partial, because it still cannot guarantee that the given library will be loaded.

The new situation is that it's difficult to check if a path exists on macOS, because for some libraries the file system check will no longer work.
It's uncertain to know if the check will work for a given path or not.

I'm OK to bring the file access check back and adjust the fix in bug 1652956, if we can find a way to distinguish the system path from a regular path.

Hopefully the suggested test in bug 1648836 will give us additional information.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Mike Hommey [:glandium] from comment #8)

I think this was an incorrect decision. The check for the file should be done at a higher level.

I disagree.

My point was:

If application code expects to see a failure, if a given file doesn't exist - then the application code should directly check if the file exists or not. It shouldn't rely on the failure result of a more complex function, which might be influenced by additional factors, besides the existence of the specific file.