Skip to content

Fix loading of R2R assemblies from byte arrays/streams #74118

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.

Sign up for GitHub

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

Merged
VSadov merged 7 commits into from
Aug 20, 2022
Merged

Fix loading of R2R assemblies from byte arrays/streams #74118

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1ea552a
enable test
VSadov Aug 17, 2022
21a4bdb
flat layout is ok if it is not a file
VSadov Aug 17, 2022
17cbdf6
do not convert non-file layouts on OSX
VSadov Aug 17, 2022
c2061d9
run some code when loading from byte array
VSadov Aug 18, 2022
4be4117
use flat layout on all platforms
VSadov Aug 19, 2022
c5dc71e
Suggestion from PR feedback
VSadov Aug 19, 2022
680b317
typo
VSadov Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/coreclr/vm/peimagelayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ PEImageLayout* PEImageLayout::LoadConverted(PEImage* pOwner)
// ConvertedImageLayout may be able to handle them, but the fact that we were unable to
// load directly implies that MAPMapPEFile could not consume what crossgen produced.
// that is suspicious, one or another might have a bug.
_ASSERTE(!pFlat->HasReadyToRunHeader());
_ASSERTE(!pOwner->IsFile() || !pFlat->HasReadyToRunHeader());
#endif

if (!pFlat->HasReadyToRunHeader() && !pFlat->HasWriteableSections())
Expand All @@ -94,6 +94,17 @@ PEImageLayout* PEImageLayout::LoadConverted(PEImage* pOwner)
return pFlat.Extract();
}

#ifdef TARGET_OSX
// We need to allocate executable memory on OSX in order to do relocation of R2R code.
// Converted layout currently uses VirtualAlloc, so it will not work.
Copy link
Member

@janvorli janvorli Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand this comment. Why would allocating executable memory not work on OSX?

Copy link
Member Author

@VSadov VSadov Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would work if correct APIs are used. It is probably not as simple as just passing MAP_JIT to VirtualAlloc or just replacing VirtualAlloc with another call.
I want to backport this to 7.0, so I'd like to not do a lot of changes to how we convert layouts, which would carry extra risk to scenarios that are not being fixed.

The goal is to make sure that assemblies can be loaded from byte arrays even if they contain R2R code.
Whether the R2R is actually enabled is of less importance. It is a relatively rare case and a reflection scenario with unclear benefits from R2R.

Linux part was just tweaking the assert, but for OSX enabling R2R needs more changes. I think it is ok to do those changes in 8.0

Copy link
Member

@janvorli janvorli Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the change is fine as is.
Just FYI though, the VirtualAlloc sets MAP_JIT automatically for executable memory allocations. Either it gets memory from the g_executableMemoryAllocator.AllocateMemory or it sets the special flag that results in MAP_JIT addition during memory reservation:

runtime/src/coreclr/pal/src/map/virtual.cpp

Lines 923 to 927 in 6f69dde

if ((flProtect & 0xff) == PAGE_EXECUTE_READWRITE)
{
flAllocationType |= MEM_RESERVE_EXECUTABLE;
}
pRetVal = ReserveVirtualMemory(pthrCurrent, (LPVOID)StartBoundary, MemSize, flAllocationType);

Copy link
Member Author

@VSadov VSadov Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that I could just pass PAGE_EXECUTE_READWRITE to VirtualAlloc on OSX?

Copy link
Member

@janvorli janvorli Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member Author

@VSadov VSadov Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried going that path (setting PAGE_EXECUTE_READWRITE), but I think I also need to toggle PAL_JitWriteProtect when copying sections and then the change started making me worried about the complexity and possible bugs.

I think I will leave that for after-7.0, we do not have to do this now.

Copy link
Member

@janvorli janvorli Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right (for both)

Copy link
Member Author

@VSadov VSadov Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've logged #74203 to follow up with enabling R2R on OSX

// Do not convert byte array images on OSX for now (that will disable R2R)
// TODO: consider relaxing this in the future.
if (!pOwner->IsFile())
{
return pFlat.Extract();
}
#endif

return new ConvertedImageLayout(pFlat);
}

Expand Down
5 changes: 2 additions & 3 deletions src/tests/readytorun/tests/main.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,9 +513,8 @@ static void RunAllTests()
Console.WriteLine("RVAFieldTest");
RVAFieldTest();

// Disable for https://github.com/dotnet/runtime/issues/71507
// Console.WriteLine("TestLoadR2RImageFromByteArray");
// TestLoadR2RImageFromByteArray();
Console.WriteLine("TestLoadR2RImageFromByteArray");
TestLoadR2RImageFromByteArray();

Console.WriteLine("TestILBodyChange");
TestILBodyChange();
Expand Down