On 13.02.2025 12:01, LIU Hao wrote:
在 2025-02-13 18:17, Jacek Caban 写道:This isn't entirely true for other linkers. While those linkers do place .idata sections, including the IAT itself, in .rdata, they don’t use .idata sections for delay imports. Instead, they implement delay loading within the linker itself rather than relying on special delay import libraries, placing the delay IAT in the .data section.I’ve seen a blog claim that MSVC makes them read-only, but in my testing with modern MSVC, that doesn’t appear to be the case, perhaps they realized it wasn’t a good fit and reverted the change? I’m not sure.MSVC does make IAT read-only, as in this example: https://gcc.godbolt.org/z/Msfqhs7x3
Your example shows the regular IAT, which should indeed be read-only. That's not the case for delay load IAT.
Also my patch for binutils took LLD as a reference, as well as MSVC.
Again, that's not the case for delay load IAT, see DelayLoadContents::getDataChunks() in LLD (IAT is stored in addresses vector).
I say it’s not a good fit because changing permissions like that is generally a bad idea. Another thread might be executing code that relies on data in the same page remaining read-only or might be performing similar permission changes for other reasons. Taking a lock only protects against concurrent __delayLoadHelper2 calls.It's actually a known issue, also referenced in the article in The Old New Thing, quoted in the commit message.It also explains why a static lock is necessary here: The unprotect operation and write operation must happen atomically, so another thread will not make the IAT read-only in between.
Some code out of our control may do a similar unprotect+protect dance for another reason.
If you have Visual Studio installed, you can also see that Microsoft code does similar stuff in 'C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\delayhlp.cpp'; however their code checks for image load config, which binutils does not support at the moment.
I can't look at that, if I did I wouldn't be allowed to work on relevant Wine code, we're pretty strict about that for good reasons.
Wine provides its own __delayLoadHelper2 implementation, which simply forwards the call to the public ResolveDelayLoadedAPI function. I’m attaching a hack that does the same for mingw-w64 (since it’s a Windows 8 API, it’s not something mingw-w64 could use by default). ResolveDelayLoadedAPI doesn’t seem to perform the protection change. at least in this case, though maybe there’s a way to trigger it? Either way, the test case crashes. The only workaround would be modifying Wine’s __delayLoadHelper2 to duplicate ResolveDelayLoadedAPI inside a static library and add the necessary permission changes.A writeable IAT is a conspicuous target for code injection.If MSVC makes it read-only, LLD makes it read-only, and there have been few issues with MSVC in practice (see the Old New Thing article), then there's no reason for us to not do that; do you agree?
It makes a lot of sense for regular IATs. Delay load IATs are a separate thing, both on MSVC and LLD.
And MSYS2 have also updated their binutils since about a week ago, and there do not seem to be issues, either.
I guess none of its binaries use delay load functionality. That's not really surprising, while we have support for generating them in mingw-w64-crt, we skip them in make install, so using them is rather tricky.
That’s unfortunate. If anyone has a better idea, please let me know. Ideally, I’d like to restore the previous behavior for delay load imports. Wine doesn’t use dlltool to create delay load import libraries; it generates them itself. In theory, this gives Wine more flexibility, but I haven’t found a way to make it work.SetEntryHookBypass: + while(InterlockedExchange(&gDelayLoadLock, 1) != 0) + Sleep(100);You could just use a critical section.The reason is that a critical section can't be initialized statically. This lock is only taken once for each symbol, so I suspect we'd better not introduce extra complexity.
static CRITICAL_SECTION cs = { .LockCount = -1 };
should do the trick.
Thanks,
Jacek
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
