drop direct hooking into wininet DLL and use delay-loading instead #2513
Conversation
src/Makefile.in
Outdated
| rufus-net.o: net.c | ||
| $(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(rufus_CFLAGS) $(CFLAGS) -c -o rufus-net.o `test -f 'net.c' || echo '$(srcdir)/'`net.c | ||
| $(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(rufus_CFLAGS) $(CFLAGS) -c -o rufus-net.o -isystem ../include `test -f 'net.c' || echo '$(srcdir)/'`net.c |
There was a problem hiding this comment.
we can also create small header that undefined DECLSPEC_IMPORT and redefined it and include it before wininet.h , uxtheme.h etc..... instead link external winnt.h with -isystem what's better up to you I will do it
ifdef DECLSPEC_IMPORT
#undef DECLSPEC_IMPORT
#endif // DECLSPEC_IMPORT
#ifndef DECLSPEC_IMPORT
#if (defined (__i386__) || defined (__ia64__) || defined (__x86_64__) || defined (__arm__) || defined(__aarch64__)) && !defined (__WIDL__)
#if defined(_MSC_VER)
#define DECLSPEC_IMPORT __declspec(dllimport)
#elif defined(__GNUC__)
#define DECLSPEC_IMPORT __attribute__((visibility ("hidden")))
#endif
#else
#define DECLSPEC_IMPORT
#endif
#endif
|
Last time I tried to delay load Maybe the MinGW devs fixed this since, but if they haven't, we're going to have a problem... |
|
delay-loading DLL in MSY2 has problem because wrong definition in mingw32\include\
if this is Rufus exits before it launches I fix this problem in this pull request example the definition of BOOLAPI expanded to the definition of INTERNETAPIX expanded to the definition of DECLSPEC_IMPORT expanded to the problem is they define DECLSPEC_IMPORT as __declspec (dllimport) but it should define as look at https://gcc.gnu.org/wiki/Visibility cane you clone my MINGW32 brunch and test an build of rufus.exe |
|
lets take another example THEMEAPI expanded to DECLSPEC_IMPORT expanded to but it shoud to be So I correct this in winnt.h and link it with gcc from current source code by add parameter to c -o rufus-net.o -isystem ../include and but the correct winnt.h there so we just need to correct the definition of DECLSPEC_IMPORT and link it |
|
I'll test these changes when I get a chance (it might be some time), but I can only accept this if we find a better way than duplicating |
Test it as you like I tested it very well and take your time
There is multiple solutions my be you don't read this post carefully If you look at the definition I mention it before DECLSPEC_IMPORT is applied only if not defined so we remove it's definition I remove the 'winnt.h' patch header and add small header that fix the problem |
Yes, I'm afraid I'm busy with other things (and might be for some time) so I don't have the scope to look closely at your patch RN. I apologize for that. The one thing I will say though is, if we believe that the MinGW headers are broken and should be fixed, shouldn't we be pushing for a MinGW fix upstream? As far as I understand it, you are stating that the MinGW folks didn't follow the visibility settings for apps and DLLs that the gcc folks say should be used for Windows, so, IMO, and especially considering that I did experience breakage when trying to delay load the DLLs, this is a MinGW bug that should be fixed upstream. So I'd rather report this and wait until it is fixed in MinGW (the MinGW people are usually pretty fast at fixing these kind of issues, and since I don't care about people trying to build Rufus outside of msys2, which is where MinGW fixes appear first, and since all Rufus builds are made using latest msys2, we'll get the changes as soon as they are available). This would avoid trying to fix the headers ourselves and I would prefer not adding workarounds for elements that could and should be fixed directly in the upstream projects. So, unless you want to report this yourself to MinGW, I don't mind doing so when I get a chance, and I will most likely wait until there's a version of MinGW/msys2 where we don't have to apply this workaround to reinstate delay loading for wininet and other DLLs in Rufus. But this does not mean that this should stall your darkmode work, as, even if we don't go with this exact workaround, you can consider that delay loading will be fixed one way or another, and move ahead with it. Again, it will probably be some time before I can start to review it in earnest anyway, so you should have plenty of time. At any rate, you have my thanks for figuring out the MinGW delay loading issue that had been annoying me for some time. Hopefully, this can be fixed upstream (as it should) so that other people don't run into it as well. |
|
I'm afraid I'm still seeing the original issue I was seeing, regardless of whether Please bear in mind (and this is something that I did not specify in my note at the time) that you only get the MinGW issue with the 32-bit version, and not the 64-bit version. At the time I was actually only publishing the MinGW 32-bit version for both x86_64 and x86_32 versions of Windows, so that's why this is not explicitly mentioned. But even though the 64-bit MinGW compiled version of Rufus appears to work, and I have switched to publishing both an x86_32 and an x86_64 version of Rufus for download, I still don't want to produce a 32-bit executable that fails to launch when run on 64-bit platforms. Can you please confirm that you also see the issue when trying to run the 32-bit MinGW version on 64-bit Windows, even with your patch applied? I'll see if I can look further into the causes of the crash for the 32-bit MinGW version (I haven't really tried debugging that crash... maybe the cause will become obvious when viewed through |
no Rufus work fine in my windows environment
I tested both uxtheme and winint
I know that so i named my new forked brunch MinGW32
how did you go that you must do that directly before |
|
Can you please confirm that you cloned my MinGW32 brunch and the Refuse failed to started from this Build in 64 bit environment |
I actually modified the MinGW But now that I look at it more closely, I realize that I had only modified the |
|
I have now reported the issue to the MinGW mailing list: |
good but link to this issue written in wrong way about https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-headers-git my be they can create patch for msys2 faster than MINGW folk and you need to report that not only winnet.dll that has a problem but uxtheme.dll also |
|
I'm pretty confident the wininet, virtdisk and uxtheme issues are all related, so I don't think it's necessary to report the list of all the libraries affected. As a matter of fact, given the nature of the issue (NULL pointer deref due to the linker wrongly believing that some function calls are unreferenced) it should be assumed, from the bug report, that if it happens for one DLL, then potentially all DLLs are affected. I am still in the process of reporting this to the binutils folks, since I agree that it does look like a linker bug, but since I expect that a fix now will take a bit longer than if it was a pure MinGW issue, I am going to push a slightly modified version of your patch that applies the This will close this PR, but I will continue to report here about the status of the bug and I will be fine with accepting a similar workaround for |
I'm not sure it's linker bug my first investigate in the problem I found the root of issue is use new calling API instead of WINAPI https://wwwdisc.chimica.unipd.it/luigino.feltre/pubblica/unix/win32_2/dll/make.html I also investigate what Jeremy say in MinGW mailing list about lld can't link .lib generated by gnu dlltools because of dlltool places the addresses that are updated at runtime within .idata, and GNU ld normally links .idata as writable, while LLD and MS link.exe normally has read-only .idata (and places the addresses that will be updated at runtime by the delay loading mechanism in .data instead). you can read about this here https://stackoverflow.com/a/63559914/3784149
no problem now I will move to work in DarkMode with this temporary solution that I found it the best solution from the beginning |
I look to the repository to make some theme with visual studio and I did not know there is msys2 build it seems that GCC bugs
is follow me every where so to use uxtheme without direct hooking we must solve this issue first because I don't like hooking into DLL and undocumented functions like AllowDarkModeForWindow,ShouldAppsUseDarkMode,GetImmersiveColor etc....
fix #2272 and #1877