#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

Description

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 15 months ago.

Download all attachments as: .zip

Change History (7)

Changed 15 months ago by lodle2

comment:1 Changed 15 months ago by lodle2

Commit causing issue: #68962

comment:3 Changed 15 months 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 15 months 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 15 months 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 15 months 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.

Note: See TracTickets for help on using tickets.