-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Allow execution of R2R code in singlefile app on windows. #40104
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
Conversation
c5ac18a to
c4997aa
Compare
|
Timing execution of HelloWorld app === baseline (not singlefile, self-contained app): real 0m0.954s === singlefile app before this change: real 0m1.216s === singlefile app after this change: real 0m0.909s |
|
Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar, @agocke |
|
I think this is ready to be reviewed. |
|
cc:@agocke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be leaving any pages mapped as RWX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen this used in other cases.
| MemAccessControl = PAGE_EXECUTE_READWRITE; |
, so I assumed we may need to keep fidelity with what PE says - if it is RWX section then it should be mapped to RWX memory.
In reality, I think I have never seen RWX actually happening here. I think R2R code is always RX, perhaps intentionally.
I think for our purposes we could just handle RWX case as RX here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows PE loader ignores executable permissions for IL-only images. It will never map executable sections in IL images as executable, even if the PE file asks for it.
This was defense-in-depth security mitigation, implemented long time in .NET Framework days. It is probably not as important now compared to how important it used to be, but it would not hurt to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memory must be executable so that we could execute code. It does not need to be writeable though. Not for R2R stuff.
I have changed this to PAGE_EXECUTE_READ for executable sections regardless of writeability.
src/coreclr/src/vm/peimagelayout.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to enable loading R2R images via e.g. Load(byte[]) ? I am not sure whether it is something we want to enable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the concern that we would do redundant relocations or that the R2R code might be made to run via reflection?
Assuming that R2R code does the same thing as corresponding IL, I do not see problems with the latter. Perhaps even a good thing.
That is - if that actually works.
I did not think that Load(byte[]) might hit this codepath. I will have to check what really happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked Load(byte[]) scenario and it appears to not using this codepath. Load(byte[]) creates RawImageLayout and that does not come here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConvertedImageLayout is used explicitly when loading from a Bundle.
There is also another codepath that could lead to creating ConvertedImageLayout. I am not sure if it is used and in what cases.
It should be possible to remember that the file was loaded form a bundle and perform the above just in those cases to be sure it does not enable more scenarios than needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added m_isInBundle to ConvertedImageLayout - to make sure this does not get enabled in other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory it is possible that ConvertedImageLayout could be used for other purposes, but even if it happens it seems to be rare. I have made a trial change where creating ConvertedImageLayout not for a purpose of a file in a bundle would assert and throw, and the change passed all CI tests. (#40403)
Anyways, we now should be relocating only when ConvertedImageLayout was created for a file in a bundle. If the other scenarios are reachable, they will still retain previous behavior.
…e. (otherwise R2R stays disabled)
jkotas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks!!! |
* Keep executable PE sections executable in LayoutILOnly. * Install unwind handlers if present * do not do RtlAddFunctionTable on x86 * Check for Cor header before checking for R2R header. * Delete function table in image dtor * Avoid PAGE_EXECUTE_READWRITE, we should not need writeable for R2R * Do relocations in ConvertedImageLayout only if the file is in a bundle. (otherwise R2R stays disabled)
The following changes were made:
RtlAddFunctionTable/RtlDeleteFunctionTablefor non-x86 unwind).Fixes:#39409