Skip to content
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

Jump to bottom

PE loader/image cleanups. No-copy mapping of R2R PEs on Windows. #61938

Merged
merged 14 commits into from
Dec 14, 2021
Merged

PE loader/image cleanups. No-copy mapping of R2R PEs on Windows. #61938

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
515a1b3
removed IMAGE_MAPPED
VSadov Nov 20, 2021
0178294
removed RawImageLayout
VSadov Nov 26, 2021
49c759f
delete MappedImageLayout
VSadov Nov 27, 2021
519fe5c
use mem-mapping on Windows
VSadov Dec 2, 2021
00ae715
tweaks and touchups
VSadov Dec 10, 2021
230e9ef
a few cleanups
VSadov Dec 10, 2021
3e2c617
comments
VSadov Dec 10, 2021
23945fb
move EnsureLoaded into Assemby::Init
VSadov Dec 11, 2021
be5f1fe
A fix for IsDynamic() case.
VSadov Dec 11, 2021
2bf609d
fix for preferred base, if used.
VSadov Dec 13, 2021
1e0a933
disable failing scenario
VSadov Dec 13, 2021
5cc040e
PR feedback
VSadov Dec 13, 2021
30be65f
Typo (CENTINEL --> SENTINEL)
VSadov Dec 14, 2021
473f734
added a bug link to a disabled test scenario + couple comment tweaks
VSadov Dec 14, 2021
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
6 changes: 1 addition & 5 deletions src/coreclr/debug/daccess/request.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1344,11 +1344,7 @@ ClrDataAccess::GetMethodDescName(CLRDATA_ADDRESS methodDesc, unsigned int count,
if (pMD->IsLCGMethod() || pMD->IsILStub())
{
// In heap dumps, trying to format the signature can fail
// in certain cases because StoredSigMethodDesc::m_pSig points
// to the IMAGE_MAPPED layout (in the PEImage::m_pLayouts array).
// We save only the IMAGE_LOADED layout to the heap dump. Rather
// than bloat the dump, we just drop the signature in these
// cases.
// in certain cases.

str.Clear();
TypeString::AppendMethodInternal(str, pMD, TypeString::FormatNamespace|TypeString::FormatFullInst);
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/inc/pedecoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,6 @@ class PEDecoder
BOOL IsILOnly() const;
CHECK CheckILOnly() const;

void LayoutILOnly(void *base, bool enableExecution) const;

// Strong name & hashing support

BOOL HasStrongNameSignature() const;
Expand Down Expand Up @@ -348,6 +346,7 @@ class PEDecoder
IMAGE_SECTION_HEADER *OffsetToSection(COUNT_T fileOffset) const;

void SetRelocated();
IMAGE_NT_HEADERS* FindNTHeaders() const;

private:

Expand All @@ -364,7 +363,6 @@ class PEDecoder

static PTR_IMAGE_SECTION_HEADER FindFirstSection(IMAGE_NT_HEADERS * pNTHeaders);

IMAGE_NT_HEADERS *FindNTHeaders() const;
IMAGE_COR20_HEADER *FindCorHeader() const;
READYTORUN_HEADER *FindReadyToRunHeader() const;

Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/inc/pedecoder.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ inline PEDecoder::PEDecoder(PTR_VOID mappedBase, bool fixedUp /*= FALSE*/)
{
CONSTRUCTOR_CHECK;
PRECONDITION(CheckPointer(mappedBase));
PRECONDITION(CheckAligned(mappedBase, GetOsPageSize()));
PRECONDITION(PEDecoder(mappedBase,fixedUp).CheckNTHeaders());
THROWS;
GC_NOTRIGGER;
Expand Down Expand Up @@ -172,7 +171,6 @@ inline HRESULT PEDecoder::Init(void *mappedBase, bool fixedUp /*= FALSE*/)
NOTHROW;
GC_NOTRIGGER;
PRECONDITION(CheckPointer(mappedBase));
PRECONDITION(CheckAligned(mappedBase, GetOsPageSize()));
PRECONDITION(!HasContents());
}
CONTRACTL_END;
Expand Down
6 changes: 2 additions & 4 deletions src/coreclr/inc/vptr_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,12 @@ VPTR_CLASS(TailCallStubManager)
#endif
VPTR_CLASS(CallCountingStubManager)
VPTR_CLASS(PEAssembly)

VPTR_CLASS(PEImageLayout)
VPTR_CLASS(RawImageLayout)
VPTR_CLASS(ConvertedImageLayout)
VPTR_CLASS(MappedImageLayout)
#if !defined(TARGET_UNIX)
VPTR_CLASS(LoadedImageLayout)
#endif // !TARGET_UNIX
VPTR_CLASS(FlatImageLayout)

#ifdef FEATURE_COMINTEROP
VPTR_CLASS(ComMethodFrame)
VPTR_CLASS(ComPlusMethodFrame)
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/pal/src/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2043,9 +2043,9 @@ MAPmmapAndRecord(

// Set the requested mapping with forced PROT_WRITE to ensure data from the file can be read there,
// read the data in and finally remove the forced PROT_WRITE
if ((mprotect(pvBaseAddress, len + adjust, prot | PROT_WRITE) == -1) ||
if ((mprotect(pvBaseAddress, len + adjust, PROT_WRITE) == -1) ||
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious - why was this change necessary?

Copy link
Member Author

@VSadov VSadov Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Without this change if prot is executable, the prot | PROT_WRITE becomes WX. mprotect will accept that, but pread will fail and we would bail from this method. This would always happen in an R2R PE, since .text is executable.

We then would go on the path of doing layout via copying, which would work, but often end up not in the desired location and later fail when applying relocations, where we had similar WX issue. Failed relocations would result in R2R disabled. The code would still run - in pure IL/JIT mode.

Ultimately we would have R2R enabled on OSX only when we are very lucky.

(pread(fd, pvBaseAddress, len + adjust, offset - adjust) == -1) ||
(((prot & PROT_WRITE) == 0) && mprotect(pvBaseAddress, len + adjust, prot) == -1))
(mprotect(pvBaseAddress, len + adjust, prot) == -1))
{
palError = FILEGetLastErrorFromErrno();
}
Expand Down
12 changes: 10 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/R2RPEBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,17 +683,25 @@ public static PEHeaderBuilder Create(Subsystem subsystem, TargetDetails target)
ulong imageBase = is64BitTarget ? PE64HeaderConstants.DllImageBase : PE32HeaderConstants.ImageBase;

int fileAlignment = 0x200;
if (!target.IsWindows && !is64BitTarget)
if (target.IsWindows || !is64BitTarget)
{
// To minimize wasted VA space on 32-bit systems, align file to page boundaries (presumed to be 4K)
//
// On Windows we use 4K file alignment, per requirements of memory mapping API (MapViewOfFile3, et al).
Copy link
Member

Choose a reason for hiding this comment

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

Nit - according to the above condition this section is used both on Windows and on 32-bit Unix (most notably Linux on arm I guess); please consider making the comment more precise.

// We also want files always accepted by the native loader, thus we can't use the same approach as on Unix.
fileAlignment = 0x1000;
}

int sectionAlignment = 0x1000;
if (!target.IsWindows && is64BitTarget)
Copy link
Member

Choose a reason for hiding this comment

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

The condition !target.IsWindows && is64BitTarget is apparently a negation of the condition target.IsWindows || !is64BitTarget but it takes a bit of Boolean algebra to double-check that. Could we use an if-else construct or a temporary boolean variable with a sufficiently descriptive name, e.g. isWindowsOr32Bit?

{
// On Linux, we must match the bottom 12 bits of section RVA's to their file offsets. For this reason
// On 64bit Linux, we must match the bottom 12 bits of section RVA's to their file offsets. For this reason
// we need the same alignment for both.
//
// In addition to that we specify section RVAs to be at least 64K apart, which is > page on most systems.
// It ensures that the sections will not overlap when mapped from a singlefile bundle, which introduces a sub-page skew.
//
// Such format would not be accepted by OS loader on Windows, but it is not a problem on Unix.
sectionAlignment = fileAlignment;
}

Expand Down
83 changes: 3 additions & 80 deletions src/coreclr/utilcode/pedecoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,10 @@ CHECK PEDecoder::CheckSection(COUNT_T previousAddressEnd, COUNT_T addressStart,
CHECK(alignedSize >= VAL32(pNT->OptionalHeader.SizeOfImage));

// Check expected alignments
#if TARGET_WINDOWS
// R2R files on Unix do not align section RVAs
Copy link
Member

Choose a reason for hiding this comment

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

Typo - should this be 'R2R files on Windows'?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, in a way the comment does hold on Unix but it's strange that it's in a TARGET_WINDOWS conditional compilation block.

Copy link
Member Author

@VSadov VSadov Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

On Windows we make section RVAs always a multiple of SectionAlignment. That is not an explicit requirement in the documentation, but it looks like OS loader expects that and our own validation here expects that too.

That is not the case on Unix where we keep lower 16bit of the RVA intact. I do not know why this asserts is not triggered on Unix. It should. When I tried using the same approach on Windows as on Unix, this assert was triggered.

Perhaps this style of assert does not run on Unix? (that would be mildly disturbing).

Anyways - I think the assert should be only enabled on Windows, where the condition is expected. At least as a matter of documenting the expectations.

Copy link
Member Author

@VSadov VSadov Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps I should use a better terminology to make it more clear.

I.E. on Windows section start RVAs are multiples of SectionAlignment. On Unix this is not true.

Any suggestions?

(same applies to having gaps between sections when loaded - Windows PEs are not expected to have gaps, Unix PEs may have gaps)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I guess that probably the only cause of my confusion was the wording - having a comment about Unix behavior under a conditional compilation section TARGET_WINDOWS at the very least looks somewhat surprising. Perhaps changing the wording to something like 'On Windows the sections are aligned blah blah blah (in contrast to Unix where they're not blah blah blah)' would be more than sufficient to address my suggestion.

CHECK(CheckAligned(addressStart, VAL32(pNT->OptionalHeader.SectionAlignment)));
#endif
CHECK(CheckAligned(offsetStart, VAL32(pNT->OptionalHeader.FileAlignment)));
CHECK(CheckAligned(offsetSize, VAL32(pNT->OptionalHeader.FileAlignment)));

Expand Down Expand Up @@ -1659,86 +1662,6 @@ CHECK PEDecoder::CheckILOnlyEntryPoint() const
}
#endif // TARGET_X86

#ifndef DACCESS_COMPILE

void PEDecoder::LayoutILOnly(void *base, bool enableExecution) const
{
CONTRACT_VOID
{
INSTANCE_CHECK;
PRECONDITION(CheckZeroedMemory(base, VAL32(FindNTHeaders()->OptionalHeader.SizeOfImage)));
// Ideally we would require the layout address to honor the section alignment constraints.
// However, we do have 8K aligned IL only images which we load on 32 bit platforms. In this
// case, we can only guarantee OS page alignment (which after all, is good enough.)
PRECONDITION(CheckAligned((SIZE_T)base, GetOsPageSize()));
THROWS;
GC_NOTRIGGER;
}
CONTRACT_END;

// We're going to copy everything first, and write protect what we need to later.

// First, copy headers
CopyMemory(base, (void *)m_base, VAL32(FindNTHeaders()->OptionalHeader.SizeOfHeaders));

// Now, copy all sections to appropriate virtual address

IMAGE_SECTION_HEADER *sectionStart = IMAGE_FIRST_SECTION(FindNTHeaders());
IMAGE_SECTION_HEADER *sectionEnd = sectionStart + VAL16(FindNTHeaders()->FileHeader.NumberOfSections);

IMAGE_SECTION_HEADER *section = sectionStart;
while (section < sectionEnd)
{
// Raw data may be less than section size if tail is zero, but may be more since VirtualSize is
// not padded.
DWORD size = min(VAL32(section->SizeOfRawData), VAL32(section->Misc.VirtualSize));

CopyMemory((BYTE *) base + VAL32(section->VirtualAddress), (BYTE *) m_base + VAL32(section->PointerToRawData), size);

// Note that our memory is zeroed already, so no need to initialize any tail.

section++;
}

// Apply write protection to copied headers
DWORD oldProtection;
if (!ClrVirtualProtect((void *) base, VAL32(FindNTHeaders()->OptionalHeader.SizeOfHeaders),
PAGE_READONLY, &oldProtection))
ThrowLastError();

// Finally, apply proper protection to copied sections
for (section = sectionStart; section < sectionEnd; section++)
{
// Add appropriate page protection.
DWORD newProtection;
if (!enableExecution)
{
if (section->Characteristics & IMAGE_SCN_MEM_WRITE)
continue;

newProtection = PAGE_READONLY;
}
else
{
newProtection = section->Characteristics & IMAGE_SCN_MEM_EXECUTE ?
PAGE_EXECUTE_READ :
section->Characteristics & IMAGE_SCN_MEM_WRITE ?
PAGE_READWRITE :
PAGE_READONLY;
}

if (!ClrVirtualProtect((void*)((BYTE*)base + VAL32(section->VirtualAddress)),
VAL32(section->Misc.VirtualSize),
newProtection, &oldProtection))
{
ThrowLastError();
}
}

RETURN;
}

#endif // #ifndef DACCESS_COMPILE

bool ReadResourceDirectoryHeader(const PEDecoder *pDecoder, DWORD rvaOfResourceSection, DWORD rva, IMAGE_RESOURCE_DIRECTORY_ENTRY** ppDirectoryEntries, IMAGE_RESOURCE_DIRECTORY **ppResourceDirectory)
{
Expand Down
27 changes: 17 additions & 10 deletions src/coreclr/vm/assembly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,17 @@ void Assembly::Init(AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocat
m_pClassLoader = new ClassLoader(this);
m_pClassLoader->Init(pamTracker);

if (GetManifestFile()->IsDynamic())
PEAssembly* pPEAssembly = GetManifestFile();

// "Module::Create" will initialize R2R support, if there is an R2R header.
// make sure the PE is loaded or R2R will be disabled.
pPEAssembly->EnsureLoaded();

if (pPEAssembly->IsDynamic())
// manifest modules of dynamic assemblies are always transient
m_pManifest = ReflectionModule::Create(this, GetManifestFile(), pamTracker, REFEMIT_MANIFEST_MODULE_NAME);
m_pManifest = ReflectionModule::Create(this, pPEAssembly, pamTracker, REFEMIT_MANIFEST_MODULE_NAME);
else
m_pManifest = Module::Create(this, mdFileNil, GetManifestFile(), pamTracker);
m_pManifest = Module::Create(this, mdFileNil, pPEAssembly, pamTracker);

FastInterlockIncrement((LONG*)&g_cAssemblies);

Expand All @@ -208,15 +214,16 @@ void Assembly::Init(AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocat
// loading it entirely.
//CacheFriendAssemblyInfo();

if (IsCollectible())
if (IsCollectible() && !pPEAssembly->IsDynamic())
Copy link
Member

Choose a reason for hiding this comment

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

Why are dynamic assemblies excluded here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dynamic assemblies do not have PE layouts. The GetLoadedImageContents below would return NULL.

{
COUNT_T size;
BYTE *start = (BYTE*)m_pManifest->GetPEAssembly()->GetLoadedImageContents(&size);
if (start != NULL)
{
GCX_COOP();
LoaderAllocator::AssociateMemoryWithLoaderAllocator(start, start + size, m_pLoaderAllocator);
}
BYTE* start = (BYTE*)pPEAssembly->GetLoadedImageContents(&size);

// We should have the content loaded at this time. There will be no other attempt to associate memory.
_ASSERTE(start != NULL);

GCX_COOP();
LoaderAllocator::AssociateMemoryWithLoaderAllocator(start, start + size, m_pLoaderAllocator);
}

{
Expand Down
7 changes: 3 additions & 4 deletions src/coreclr/vm/assemblyname.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,9 @@ FCIMPL1(Object*, AssemblyNameNative::GetFileInformation, StringObject* filenameU
SString sFileName(gc.filename->GetBuffer());
PEImageHolder pImage = PEImage::OpenImage(sFileName, MDInternalImport_NoCache);

// Load the temporary image using a flat layout, instead of
// waiting for it to happen during HasNTHeaders. This allows us to
// get the assembly name for images that contain native code for a
// non-native platform.
// Ask for FLAT. We will only look at metadata and release the image shortly.
// Besides we may be getting the assembly name for images that contain native code for a
// non-native platform and would end up using flat anyways.
PEImageLayout* pLayout = pImage->GetOrCreateLayout(PEImageLayout::LAYOUT_FLAT);

pImage->VerifyIsAssembly();
Expand Down
20 changes: 2 additions & 18 deletions src/coreclr/vm/assemblynative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,20 +138,8 @@ Assembly* AssemblyNative::LoadFromPEImage(AssemblyBinder* pBinder, PEImage *pIma
CONTRACT_END;

Assembly *pLoadedAssembly = NULL;

ReleaseHolder<BINDER_SPACE::Assembly> pAssembly;

// Force the image to be loaded and mapped so that subsequent loads do not
// map a duplicate copy.
if (pImage->IsFile())
{
pImage->Load();
}
else
{
pImage->LoadNoFile();
}

DWORD dwMessageID = IDS_EE_FILELOAD_ERROR_GENERIC;

// Set the caller's assembly to be CoreLib
Expand Down Expand Up @@ -251,11 +239,7 @@ extern "C" void QCALLTYPE AssemblyNative_LoadFromStream(INT_PTR ptrNativeAssembl
_ASSERTE((ptrAssemblyArray != NULL) && (cbAssemblyArrayLength > 0));
_ASSERTE((ptrSymbolArray == NULL) || (cbSymbolArrayLength > 0));

// We must have a flat image stashed away since we need a private
// copy of the data which we can verify before doing the mapping.
PVOID pAssemblyArray = reinterpret_cast<PVOID>(ptrAssemblyArray);

PEImageHolder pILImage(PEImage::LoadFlat(pAssemblyArray, (COUNT_T)cbAssemblyArrayLength));
PEImageHolder pILImage(PEImage::CreateFromByteArray((BYTE*)ptrAssemblyArray, (COUNT_T)cbAssemblyArrayLength));

// Need to verify that this is a valid CLR assembly.
if (!pILImage->CheckILFormat())
Expand Down Expand Up @@ -318,7 +302,7 @@ extern "C" void QCALLTYPE AssemblyNative_LoadFromInMemoryModule(INT_PTR ptrNativ
_ASSERTE(ptrNativeAssemblyBinder != NULL);
_ASSERTE(hModule != NULL);

PEImageHolder pILImage(PEImage::LoadImage((HMODULE)hModule));
PEImageHolder pILImage(PEImage::CreateFromHMODULE((HMODULE)hModule));

// Need to verify that this is a valid CLR assembly.
if (!pILImage->HasCorHeader())
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/domainfile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,6 @@ void DomainFile::LoadLibrary()
}
CONTRACTL_END;

GetPEAssembly()->EnsureLoaded();
}

void DomainFile::PostLoadLibrary()
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4337,8 +4337,8 @@ VOID MethodTableBuilder::InitializeFieldDescs(FieldDesc *pFieldDescList,
DWORD rva;
IfFailThrow(pInternalImport->GetFieldRVA(pFD->GetMemberDef(), &rva));

// Ensure that the IL image is loaded.
GetModule()->GetPEAssembly()->EnsureLoaded();
// The PE should be loaded by now.
_ASSERT(GetModule()->GetPEAssembly()->IsLoaded());

DWORD fldSize;
if (FieldDescElementType == ELEMENT_TYPE_VALUETYPE)
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/nativeimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,11 @@ NativeImage *NativeImage::Open(
// Composite r2r PE image is not a part of anyone's identity.
// We only need it to obtain the native image, which will be cached at AppDomain level.
PEImageHolder pImage = PEImage::OpenImage(fullPath, MDInternalImport_NoCache, bundleFileLocation);
PEImageLayout* mapped = pImage->GetOrCreateLayout(PEImageLayout::LAYOUT_MAPPED);
PEImageLayout* loaded = pImage->GetOrCreateLayout(PEImageLayout::LAYOUT_LOADED);
// We will let pImage instance be freed after exiting this scope, but we will keep the layout,
// thus the layout needs an AddRef, or it will be gone together with pImage.
mapped->AddRef();
peLoadedImage = mapped;
loaded->AddRef();
peLoadedImage = loaded;
}

if (peLoadedImage.IsNull())
Expand Down
Loading