Opened 2 years ago

Closed 5 weeks ago

#15534 closed defect (fixed)

wxWidgets resets dll search directory when it shouldnt (SetDllDirectory)

Reported by: lodle2 Owned by:
Priority: normal Milestone: 3.0.0
Component: wxMSW Version: 2.9.4
Keywords: SetDllDirectory wxDynamicLibrary Cc: robin
Blocked By: Blocking:
Patch: yes


wxDynamicLibrary::RawLoad changes the dll directory to be the directory where the exe is located however it resets it back to null where it should reset it back to the previous dll directory.

This causes desura dll's that are loaded after wxWidgets to fail due to them being located in the bin dir.

Attachments (1)

wxWidgets_setdlldirectorybug.diff download (1.4 KB) - added by lodle2 2 years ago.

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by lodle2

comment:1 Changed 2 years ago by lodle2

Commit causing issue: #68962

comment:3 Changed 2 years ago by vadz

  • Cc robin added
  • Priority changed from high to normal
  • Status changed from new to confirmed

Thanks for finding this problem and providing the fix!

Unfortunately I'm not sure that it's 100% correct: currently we break the behaviour of the programs that call SetDllDirectory(), but wouldn't we break the behaviour of all the other programs with the proposed change? As we'd be calling SetDllDirectory() with a non-NULL parameter, it wouldn't be restoring the default DLL search order which is presumably still used by the majority of programs. And I don't know how can we distinguish between the two cases, can we use GetDllDirectory() for that?

In the meanwhile, I think the best solution might be to just not call SetDllDirectory() at all, this seems a wrong function to use here and this seems to be a wrong place to use it. If it's really needed by wxPython only in the specific case described in the comment, we probably ought to add a special flag for it (wxDL_SEARCH_WXDLL_DIR?) and use it where it's needed.

Also, we could just (temporarily) append this directory to the PATH instead of using SetDllDirectory() which really should be left to the application discretion.

Robin, what do you think?

comment:4 Changed 2 years ago by lodle2

I do think we shouldnt use this at all as its not thread safe as well. A flag could work but that is beyond my scope.

comment:5 Changed 2 years ago by vadz

  • Milestone set to 3.0

Flag is not difficult to introduce (basically we'd just need to take the code calling SetDllDirectory() inside if ( flags & wxDL_SEARCH_MODULE_DIR )), but thinking more about it, it's so specific that I don't even believe it's useful to have. Frankly, I'd rather just remove this code and let wxPython do it, perhaps by just calling SetDllDirectory() once on startup. If it could help Robin, we could add static wxDynamicLibrary::MSWSetDllDirectory() or something like this, of course.

If there are no objections soon, I'm going to remove it.

comment:6 Changed 2 years ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [74937]) Don't call SetDllDirectory() when loading dynamic libraries in wxMSW.

SetDllDirectory() modifies the per-process DLL loading behaviour which is
already unexpected as it can affect other threads, running code completely
unrelated to wxWidgets, but, even worse, we can't undo its effect as calling
SetDllDirectory(NULL) as we used to discarded any changes to the DLL directory
done by the program itself, while restoring the result of GetDllDirectory()
would never restore the "compatible" algorithm for DLL search used by default.

So the simplest, and the only 100% correct solution, is to not call this
function at all from here and call it from some higher level code only, either
in the user application or wxPython itself.

Closes #15534.

comment:7 Changed 5 weeks ago by paulclinger

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.9.4 to dev-latest

Vadim, I have to re-open this as I ran into an issue and bisected it to the commit referenced in this ticket. The functionality that loads external modules was working in application with v2.9.5, but stopped working with the current master.

Here is what I think happens. My application (Lua-based) loads external modules using LoadLibraryExA, but the modules may be located in arbitrary directories, including the current directory that changes dynamically. With the code you removed the SetDllDirectory was adding the module directory to the search path, but without it the modules fail to load. Unfortunately, since I'm only working at the Lua level (it's a thin wrapper around wxwidgets, similar to wxPython), I have no way of setting SetDllDirectory and even if I had access to it, I'd still need to intercept dll loading process, which I can't do either.

(I still don't quite understand when exactly RawLoad that does SetDllDirectory call comes into play when my application (internally) calls LoadLibraryExA. If it doesn't come into play, then the lack of SetDllDirectory should make no difference, but it does make a difference.)

It's too bad that Robin didn't respond as I suspect that wxPython may suffer from the same issue. Setting SetDllDirectory on startup doesn't work in my case as the directory from which modules are loaded can be changed by the user in the application.

I agree with the comment that the restored path should probably take the "initial" path into account, but the comment in the original ticket -- "changes the dll directory to be the directory where the exe is located" -- is not correct as it changes the dll directory to where the loaded module is located, which is what Microsoft recommends (

The search path can be altered using the SetDllDirectory function. This solution is recommended instead of using SetCurrentDirectory or hard-coding the full path to the DLL.

The comment about this call being not thread safe is correct, but it would be the same result in the case of the app making the call; if needed, the app can "protect" the resource using usual mechanisms.

To summarize: I think the patch should be reverted, possibly with the addition of restoring the path based on GetDllDirectory instead of NULL. I tested the current master with the reverted patch and it works as before.

comment:8 Changed 5 weeks ago by paulclinger

  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from dev-latest to 2.9.4

I take that back. I found a way to fix this by avoiding using SetDllDirectory altogether, which keeps the default value intact. It used to work in my case because of the SetDllDirectory(NULL) call in the end, not because of the setting it when the DLL was loaded.

Note: See TracTickets for help on using tickets.