-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Windows executables: only load imported DLLs from System32 #89311
Conversation
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue DetailsBy using the /DEPENDENTLOADFLAG link.exe flag, we can tell the Windows loader to only look for referenced DLLs in the System32 directory. This prevents DLL injection, like #87495 recently fixed. This covers the main CoreCLR product DLLs and EXEs:
QuestionsIs there a better way to apply this flag in the CMake files? Initially I tried adding this to all executables by defining the flag in When I added this flag, I got this error message from LINK.exe:
How can this be worked around? Currently I have commented out applying PGO to make the build pass.
|
@elinor-fung as our inhouse DLL loading expert :-) |
For the binaries under src/native/corehost, you could put the option under exe.cmake and lib.cmake, which will be used by product executables/libraries but not tests. Do you recall which tests these were? If it is an isolated set, you could have them explicitly remove the option. We do something like that for IJW tests: runtime/eng/native/ijw/IJW.cmake Line 3 in 2470610
I believe this is because the data files used for PGO optimization need to be collected/applied withbinaries that use identical options. One option would be to do this in two stages:
@jkoritzinsky / @agocke do you know if there's a better way to do this? |
Generally when we change options in an instrumenting-incompatible way, we just disable PGO optimization as part of the change until new data has been generated and flows back into the runtime. Your idea actually sounds nicer than our usual approach. I think we should at least wait until after the snap (so wait until tomorrow) before taking a PGO-incompatible change just to simplify code flow. |
Agreed. |
Thanks for the pointer to the IJW file. I will try to do something like that for tests that break when this linker flag is applied globally. I'll un-draft this PR once it get everything building again. |
70dd9da
to
547fdc6
Compare
547fdc6
to
355b455
Compare
It looks like it was just the IJW tests in |
This looks good to me. I just want to make sure we are in a good position to get updated PGO data and re-enable PGO once this goes in. @DrewScoggins - we haven't gotten new PGO data in over a month. It looks like the internal dotnet-optimization builds have been failing or timing out. Do you know if this is a known issue or something that still needs to be investigated? |
It is currently being investigated. The current issue I am working through is related a dependency of the repo using an insecure version of a dependency. I will keep things updated as we work through the issues. |
The scenario issue is fixed, but now we are seeing an internal runtime error when doing collection. I have created the issue below to track it. |
Thanks @DrewScoggins. I see dotnet-optimization is back up and we are taking updates (#91171) again - yay! |
Thanks, @AustinWise! Merging this. |
By using the /DEPENDENTLOADFLAG link.exe flag, we can tell the Windows loader to only look for referenced DLLs in the System32 directory. This prevents DLL injection, like #87495 recently fixed.
This covers the main CoreCLR product DLLs and EXEs:
Questions
Is there a better way to apply this flag in the CMake files? Initially I tried adding this to all executables by defining the flag ineng/native/
, but some tests broke because they rely on loading DLLs from places other than System32.EDIT: I moved the setting to
eng/native/configurecompiler.cmake
and disabled the linker flag for IJW tests.When I added this flag, I got this error message from LINK.exe:How can this be worked around? Currently I have commented out applying PGO to make the build pass.EDIT: We can leave the application of PGO commented out until updated PGO files are available.