Add .mvid section to PE and remove dependency on MetadataReader #19133
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.Text; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.BuildTasks | ||
| { | ||
| public sealed class MvidReader : BinaryReader | ||
|
There was a problem hiding this comment. Are there callers that instantiate this type? If not, could this be a There was a problem hiding this comment. I'll make the constructor private. |
||
| { | ||
| public MvidReader(Stream stream) : base(stream) | ||
| { | ||
| } | ||
|
|
||
| public static Guid ReadAssemblyMvidOrEmpty(Stream stream) | ||
| { | ||
| try | ||
| { | ||
| var mvidReader = new MvidReader(stream); | ||
| return mvidReader.TryFindMvid(); | ||
| } | ||
| catch (EndOfStreamException) | ||
| { | ||
| } | ||
|
|
||
| return Guid.Empty; | ||
|
There was a problem hiding this comment. Feel like this is essentially hiding failure here. Consider using a There was a problem hiding this comment. Is the exception handler still needed? There was a problem hiding this comment. I think so. If the ref assembly is truncated (there are section headers, but no actual sections), we risk reading past the end of the file. #Resolved |
||
| } | ||
|
|
||
| public Guid TryFindMvid() | ||
| { | ||
| Guid empty = Guid.Empty; | ||
|
|
||
| // DOS Header (64), DOS Stub (64), PE Signature (4), COFF Header (20), PE Header (224) | ||
| if (BaseStream.Length < 64 + 64 + 4 + 20 + 224) | ||
| { | ||
| return empty; | ||
| } | ||
|
|
||
| // DOS Header: PE (2) | ||
| if (ReadUInt16() != 0x5a4d) | ||
| { | ||
| return empty; | ||
| } | ||
|
|
||
| // DOS Header: Start (58) | ||
| Skip(58); | ||
|
|
||
| // DOS Header: Address of PE Signature | ||
| MoveTo(ReadUInt32()); | ||
|
There was a problem hiding this comment.
It looks like we are moving to an unknown position without verifying if the destination makes sense. #Resolved There was a problem hiding this comment. Yes. That's why this logic is wrapped in a There was a problem hiding this comment.
But this method is public and it doesn't catch any exceptions. #Resolved There was a problem hiding this comment. I'd prefer to avoid first-chance exceptions if at all possible. Just read the value and check against the stream length. #Resolved |
||
|
|
||
| // PE Signature ('P' 'E' null null) | ||
| if (ReadUInt32() != 0x00004550) | ||
| { | ||
| return empty; | ||
| } | ||
|
|
||
| // COFF Header: Machine (2) | ||
| Skip(2); | ||
|
|
||
| // COFF Header: NumberOfSections (2) | ||
| ushort sections = ReadUInt16(); | ||
|
|
||
| // COFF Header: TimeDateStamp (4), PointerToSymbolTable (4), NumberOfSymbols (4) | ||
| Skip(12); | ||
|
|
||
| // COFF Header: OptionalHeaderSize (2) | ||
| ushort optionalHeaderSize = ReadUInt16(); | ||
|
|
||
| // COFF Header: Characteristics (2) | ||
| Skip(2); | ||
|
|
||
| // Optional header | ||
| Skip(optionalHeaderSize); | ||
|
|
||
| // Section headers | ||
| return FindMvidInSections(sections); | ||
| } | ||
|
|
||
| private Guid FindMvidInSections(ushort count) | ||
| { | ||
| for (int i = 0; i < count; i++) | ||
| { | ||
| // Section: Name (8) | ||
| byte[] name = ReadBytes(8); | ||
|
There was a problem hiding this comment.
Where is this helper defined? #Resolved There was a problem hiding this comment. In |
||
| if (name.Length == 8 && name[0] == '.' && | ||
| name[1] == 'm' && name[2] == 'v' && name[3] == 'i' && name[4] == 'd') | ||
|
There was a problem hiding this comment.
|
||
| { | ||
| // Section: VirtualSize (4) | ||
| uint virtualSize = ReadUInt32(); | ||
|
|
||
| // Section: VirtualAddress (4), SizeOfRawData (4) | ||
| Skip(8); | ||
|
|
||
| // Section: PointerToRawData (4) | ||
| uint pointerToRawData = ReadUInt32(); | ||
|
|
||
| // The .mvid section only stores a Guid | ||
| Debug.Assert(virtualSize == 16); | ||
|
There was a problem hiding this comment.
I think we should verify the size, rather than simply assert it. #Resolved |
||
|
|
||
| BaseStream.Position = pointerToRawData; | ||
|
There was a problem hiding this comment.
Should we use Move() here? #Resolved |
||
| byte[] guidBytes = new byte[16]; | ||
| BaseStream.Read(guidBytes, 0, 16); | ||
|
There was a problem hiding this comment.
Not checking how many bytes were read. #Resolved |
||
|
|
||
| return new Guid(guidBytes); | ||
| } | ||
| else | ||
| { | ||
|
There was a problem hiding this comment. Nit: no need for else since the if-statement returns #Resolved |
||
| // Section: VirtualSize (4), VirtualAddress (4), SizeOfRawData (4), | ||
| // PointerToRawData (4), PointerToRelocations (4), PointerToLineNumbers (4), | ||
| // NumberOfRelocations (2), NumberOfLineNumbers (2), Characteristics (4) | ||
| Skip(4 + 4 + 4 + 4 + 4 + 4 + 2 + 2 + 4); | ||
| } | ||
| } | ||
|
|
||
| return Guid.Empty; | ||
| } | ||
|
|
||
| public void Skip(int bytes) | ||
| { | ||
| BaseStream.Seek(bytes, SeekOrigin.Current); | ||
| } | ||
|
|
||
| public void MoveTo(uint position) | ||
| { | ||
| BaseStream.Seek(position, SeekOrigin.Begin); | ||
| } | ||
|
|
||
| } | ||
| } | ||
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 am not sure if this is accurate. #Closed
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.
@gafter may want to comment. From our discussion, it was ok to strip explicit method implementations.
That said, there is still a PROTOTYPE marker in this branch for this (see
GetExplicitImplementationOverrides).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.
Then let's not answer the question for now and discuss the issue offline. #Resolved
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.
Reverted the note. Keeping open question on explicit method implementations.
In reply to: 114207367 [](ancestors = 114207367)