When cloning method descs be careful with fields that have different offsets between the source and destination. #115798
Conversation
|
Tagging subscribers to this area: @mangod9 |
|
CC: @jakobbotsch - the fix for the async issue. |
VSadov
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect MethodDesc cloning when source and destination layouts differ by copying only the base portion and individually copying remaining fields.
- Add a regression test project and test to reproduce the offset-cloning issue.
- Update
AllocAndInitMethodDescChunkto memcpy only the base for non-generic methods and handle the rest manually. - Retain full memcpy for generic methods to preserve existing behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tests/Regressions/coreclr/GitHub_115667/test115667.csproj | Add project file for new regression test |
| src/tests/Regressions/coreclr/GitHub_115667/test115667.cs | Add regression test reproducing incorrect MethodDesc cloning |
| src/coreclr/vm/methodtablebuilder.cpp | Split memcpy into base-only copy for non-generic MethodDescs, then copy remainder fields |
Comments suppressed due to low confidence (2)
src/coreclr/vm/methodtablebuilder.cpp:7322
- The new
elsebranch for generic methods (full memcpy path) isn't covered by existing tests; consider adding a regression test for cloning a genericMethodDescto ensure this path remains correct.
else
src/coreclr/vm/methodtablebuilder.cpp:7303
- Using
pUnboxedMD->GetBaseSize()reads from uninitialized memory; usepMD->GetBaseSize()to determine the correct number of bytes to copy from the source.
memcpy(pUnboxedMD, pMD, pMD->GetBaseSize());
| pUnboxedMD->SetHasNonVtableSlot(); | ||
| if (bmtGenerics->GetNumGenericArgs() == 0) | ||
| { | ||
| memcpy(pUnboxedMD, pMD, pMD->GetBaseSize()); |
There was a problem hiding this comment.
This is basically the fix. memcpy(pUnboxedMD, pMD, pMD->SizeOf()); vs. memcpy(pUnboxedMD, pMD, pMD->GetBaseSize());.
memcpy(pUnboxedMD, pMD, pMD->SizeOf()); may copy a few fields to wrong locations. Then it is not enough to write correct values to the new locations, we would also need to clean up wrong locations.
It is better for the initial memcopy to do only the base portion, which is the same between different layouts. The rest stays zero-inited.
There was a problem hiding this comment.
The problem that we see now is that we are corrupting a slot that is later set via interlocked exchange, which expects 0 comparand.
Since we have junk in the slot, the exchange does not happen and the junk stays in the slot and causes crashes.
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I suspect the new test will need to be disabled in various places that do not support collectible ALCs or loading files from disk. You can check some of the other regression tests that use the CollectibleALC pattern to see where. |
| } | ||
| } | ||
|
|
||
| public class Program |
There was a problem hiding this comment.
| public class Program | |
| public class Runtime_115667 |
| System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName); | ||
| mi.Invoke(null, new object[] { System.Activator.CreateInstance(runtimeTy) }); | ||
|
|
||
| return 100; |
There was a problem hiding this comment.
This is unnecessary (and will fail a build analyzer we have in some builds). The test can return void instead.
|
addressed PR feedback |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes: #115667
When cloning methoddescs into destination that has a different layout, memcopy only the base portion, then copy remaining fields individually.
If we memcopy the whole thing, non-base fields would be copied to wrong locations. Even if we subsequently copy the non-base fields individually, the initial memcopy may corrupt destination fields that are not present in the source.
The issue appears to be not new, but somehow did not cause problems until now.