Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,26 @@ public PEHeaders(Stream peStream, int size, bool isLoadedImage)
_coffHeaderStartOffset = reader.Offset;
_coffHeader = new CoffHeader(ref reader);

if (!isCoffOnly)
if (isCoffOnly)
{
// These characteristics bits are documented in https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image
// that should be set only on image files.
const Characteristics ImageOnlyCharacteristics = Characteristics.RelocsStripped | Characteristics.ExecutableImage;
// Because COFF files do not have a magic number, perform some additional checks to catch outright invalid files.
if (_coffHeader.SizeOfOptionalHeader != 0
|| _coffHeader.PointerToSymbolTable >= actualSize
|| (_coffHeader.Characteristics & ImageOnlyCharacteristics) != 0)
{
throw new BadImageFormatException(SR.UnknownFileFormat);
}
}
else
{
_peHeaderStartOffset = reader.Offset;
_peHeader = new PEHeader(ref reader);
}

_sectionHeaders = ReadSectionHeaders(ref reader);
_sectionHeaders = ReadSectionHeaders(ref reader, actualSize, isCoffOnly);

if (!isCoffOnly)
{
Expand Down Expand Up @@ -289,10 +302,10 @@ private static void SkipDosHeader(ref PEBinaryReader reader, out bool isCOFFOnly
}
}

private ImmutableArray<SectionHeader> ReadSectionHeaders(ref PEBinaryReader reader)
private ImmutableArray<SectionHeader> ReadSectionHeaders(ref PEBinaryReader reader, int size, bool isCoffOnly)
{
int numberOfSections = _coffHeader.NumberOfSections;
if (numberOfSections < 0)
if (numberOfSections < 0 || numberOfSections * SectionHeader.Size > size - reader.Offset)
{
throw new BadImageFormatException(SR.InvalidNumberOfSections);
}
Expand All @@ -301,7 +314,12 @@ private ImmutableArray<SectionHeader> ReadSectionHeaders(ref PEBinaryReader read

for (int i = 0; i < numberOfSections; i++)
{
builder.Add(new SectionHeader(ref reader));
var sectionHeader = new SectionHeader(ref reader);
if (isCoffOnly && sectionHeader.VirtualSize != 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#112830 (comment) said that VirtualAddress is the field that must be zero in object files, but according to documentation it's VirtualSize.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds right. For VirtualAddress the doc says "For object files, this field is the address of the first byte before relocation is applied; for simplicity, compilers should set this to zero.". So yeah, we probably don't want to enforce zero for object files. Even for executable files this may be zero for some sections like .debug.

{
throw new BadImageFormatException(SR.UnknownFileFormat);
}
builder.Add(sectionHeader);
}

return builder.MoveToImmutable();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ public void Ctor_Streams()
Assert.Equal(0, s.Position);
}

[Fact]
public void Ctor_InvalidFile()
Copy link
Contributor

Choose a reason for hiding this comment

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

What case is this catching from above, and related question is what coverage do we have on the new checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It catches the "size of optional header is nonzero" case. I can add more tests.

{
using var stream = new MemoryStream(Misc.KeyPair);
Assert.Throws<BadImageFormatException>(() => new PEHeaders(stream));
}

[Fact]
public void FromEmptyStream()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -869,15 +869,5 @@ public unsafe void InvokeCtorWithIsLoadedImageAndPrefetchMetadataOptions2()
}
}
}

[Fact]
public void HasMetadataShouldReturnFalseWhenPrefetchingMetadataOfImageWithoutMetadata()
{
using (var fileStream = new MemoryStream(Misc.KeyPair))
using (var peReader = new PEReader(fileStream, PEStreamOptions.PrefetchMetadata | PEStreamOptions.LeaveOpen))
{
Assert.False(peReader.HasMetadata);
}
}
}
}
Loading