Closed Bug 511315 Opened 11 years ago Closed 10 years ago

PR_LoadLibraryWithFlags should have a way to set LOAD_WITH_ALTERED_SEARCH_PATH flag with LoadLibraryEx

Categories

(NSPR :: NSPR, enhancement, P2)

x86
Windows Server 2003
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

References

Details

Attachments

(1 file, 1 obsolete file)

PR_LoadLibraryWithFlags currently uses LoadLibrary.

It should be switched to using LoadLibraryEx so that it can take advantage of the LOAD_WITH_ALTERED_SEARCH_PATH flag . 

This would allow smoother loading of the direct-linked dependencies of DLLs.
For example, this flag allows the following case to work :

If the executable is running from C:\A.EXE , it can load a C:\DIR\B.DLL with a dependency of C:\DIR\C.DLL even if C:\DIR isn't in the PATH .
Blocks: 511312
For more info about this flag, see http://msdn.microsoft.com/en-us/library/ms682586(VS.85).aspx .

Quote :

"The LoadLibraryEx function supports an alternate search order if the call specifies LOAD_WITH_ALTERED_SEARCH_PATH and the lpFileName parameter specifies an absolute path.

Note that the standard search strategy and the alternate search strategy specified by LoadLibraryEx with LOAD_WITH_ALTERED_SEARCH_PATH differ in just one way: The standard search begins in the calling application's directory, and the alternate search begins in the directory of the executable module that LoadLibraryEx is loading."

The flag does not work with relative PATHs, per http://msdn.microsoft.com/en-us/library/ms684179(VS.85).aspx .

I think we cannot set this flag unconditionally because of binary compatibility implications. We need to create a new flag for PR_LoadLibraryWithFlags to trigger this system flag for LoadLibraryEx.
No longer blocks: 511312
Blocks: 511312
Wan-Teh,

Please review. Even if we don't immediately decide to use this to resolve bug 511312, this is probably a good thing to have in NSPR.
Attachment #395494 - Flags: review?(wtc)
Priority: -- → P2
Target Milestone: --- → 4.8.1
What's the abstraction here?
Seems to me that NSPR should attempt to provide a virtual platform, with 
platform-independent features, offered on all supported platforms, insofar 
as possible.  So, what might the equivalent functionality be for 
PR_LD_ALTERED_SEARCH_PATH on Linux or MacOSX?
Attachment #395494 - Flags: review?(wtc) → review+
Comment on attachment 395494 [details] [diff] [review]
Add PR_LD_ALTERED_SEARCH_PATH flag

r=wtc.  Please attach a new patch with the following changes
before you check it in.

>+#define PR_LD_ALTERED_SEARCH_PATH  0x10  /* equivalent to
>+                           * LOAD_WITH_ALTERED_SEARCH_PATH on Windows 

Please shorten this to PR_LD_ALT_SEARCH_PATH.

I wonder if it would look nicer to move the comment to
the previous line, like this:

/* equivalent to LOAD_WITH_ALTERED_SEARCH_PATH on Windows */
#define PR_LD_ALT_SEARCH_PATH 0x10

>+    h = LoadLibraryExW(wname, NULL, (flags & PR_LD_ALTERED_SEARCH_PATH) ?
>+                                    LOAD_WITH_ALTERED_SEARCH_PATH : 0);

Please move the third argument to the second line:

    h = LoadLibraryExW(wname, NULL,
                       (flags & PR_LD_ALT_SEARCH_PATH) ?
                       LOAD_WITH_ALTERED_SEARCH_PATH : 0);

You also need to add this function to mozilla/nsprpub/pr/src/nspr.def.
Add a new NSPR_4.8.1 section.
Comment on attachment 395494 [details] [diff] [review]
Add PR_LD_ALTERED_SEARCH_PATH flag

Ah, nspr.def doesn't need to be changed because you
didn't add a new function.  Sorry about the confusion.

To address Nelson's comment, we can make it clear that
this is a Windows-only feature:

#ifdef WIN32
/* equivalent to LOAD_WITH_ALTERED_SEARCH_PATH on Windows */
#define PR_LD_ALT_SEARCH_PATH 0x10
#endif

or without the ifdef:

/*
 * equivalent to LOAD_WITH_ALTERED_SEARCH_PATH on Windows,
 * ignored on other platforms
 */
#define PR_LD_ALT_SEARCH_PATH 0x10

Another option is to make NSPR try the alternate search path
automatically if the standard search path doesn't work.  This
has the drawback that the failure of PR_LoadLibrary will be
twice as expensive.  Some apps call PR_LoadLibrary to try
a few possible locations of a DLL, so some of those PR_LoadLibrary
calls are expected to fail, and this will hurt them.  As I
noted in bug 454508 comment 6, we can't tell by the error code
of the first LoadLibrary call whether retrying with alternate
search path will help, so we will always need to try it.
Nelson,

re: comment 3,
There is no abstraction in this case, it is just wrapping of platform-specific features. If you look at the other pre-existing PR_LD_ flags, they are platform-specific too, and apply to Unix only. My patch just followed the existing trend in adding one more platform-specific flag, but this time for Windows. This new flag is simply ignored on non-Windows platforms, just as these other pre-existing flags are ignored on non-Unix platforms.

Wan-Teh,
re: comment 4,
I can fix the indentation & comments as well as shorten the flag name. No change to nspr.def is necessary.

re: comment 5,
I think it's undesirable to have this alternate search path as the default behavior. It is likely to cause undesired effects in apps that depend on the old behavior. This is why I implemented this feature as an optional flag, and did not try to make the alternate search automatic.
Attachment #395494 - Attachment is obsolete: true
Checking in pr/include/prlink.h;
/cvsroot/mozilla/nsprpub/pr/include/prlink.h,v  <--  prlink.h
new revision: 3.16; previous revision: 3.15
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.103; previous revision: 3.102
done
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.