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?

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

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.

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

I disagree.

deleted

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

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.

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.

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