Add .mvid section to PE and remove dependency on MetadataReader #19133
Conversation
| // Section: VirtualSize (4) | ||
| uint virtualSize = ReadUInt32(); | ||
|
|
||
| // Section: VirtualAddress (4), SizeOfRawData (4) |
There was a problem hiding this comment.
@tmat One thing I noticed is that SizeOfRawData is 500. I wonder if there is a way to reduce that overhead by tweaking FileAlignment? After all, this section only contains 16 bytes. #Resolved
There was a problem hiding this comment.
Sadly, the PE spec requires the value of FileAlignment to be
... a power of 2 between 512 and 64K, inclusive. #Resolved
|
|
||
| VerifyEntryPoint(metadataOutput, expectZero: true); | ||
| VerifyMethods(metadataOutput, new[] { "C..ctor()" }); | ||
| VerifyMVID(metadataOutput, hasMvidSection: true); |
There was a problem hiding this comment.
Also, any additional ideas for validation are appreciated. The only validation I have so far is that I can extract the MVID and it matches what we got from PEReader before. And for a regular assembly, there is no .mvid section (no MVID by new method). #Resolved
There was a problem hiding this comment.
I'd also try to run peverify on the ref assembly. #Resolved
There was a problem hiding this comment.
Also add a ref assembly test to deterministic tests. #Resolved
There was a problem hiding this comment.
We already have tests that do PEVerify on ref assemblies. That's the main reason I include throw null bodies in ref assemblies.
I'll add deterministic test. #Resolved
There was a problem hiding this comment.
@tmat I added a test in deterministic tests, but note that most ref assemblies already use that option, since that is the main purpose. #Resolved
|
Tagging @heejaechang Who uses mvids in the OOP scenario. #Resolved |
| /// Extract the MVID using two different methods (PEReader and MvidReader) and compare them. | ||
| /// We only expect an .mvid section in ref assemblies. | ||
| /// </summary> | ||
| void VerifyMVID(MemoryStream stream, bool hasMvidSection) |
There was a problem hiding this comment.
Please use explicit accessibility modifier. #Resolved
There was a problem hiding this comment.
nit: VerifyMvid instead of VerifyMVID #Resolved
| { | ||
| } | ||
|
|
||
| return Guid.Empty; |
There was a problem hiding this comment.
Feel like this is essentially hiding failure here. Consider using a TryReadAssemblyMvid pattern and potentially have a wrapper named ReadAssemblyMvidOrEmpty #Resolved
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
| return FindMvidInSections(sections); | ||
| } | ||
|
|
||
| string ReadSectionName() |
There was a problem hiding this comment.
Please use explicit accessibility modifier. #Resolved
| return new string(buffer, 0, read); | ||
| } | ||
|
|
||
| Guid FindMvidInSections(ushort count) |
There was a problem hiding this comment.
Please use explicit accessibility modifier. #Resolved
| /// <summary> | ||
| /// This PEBuilder adds an .mvid section. | ||
| /// </summary> | ||
| private class ExtendedPEBuilder : ManagedPEBuilder |
There was a problem hiding this comment.
cl [](start = 16, length = 2)
sealed #Resolved
| /// <summary> | ||
| /// This PEBuilder adds an .mvid section. | ||
| /// </summary> | ||
| private class ExtendedPEBuilder : ManagedPEBuilder |
There was a problem hiding this comment.
ExtendedPEBuilder [](start = 22, length = 17)
Move to a separate file. #Resolved
| @@ -186,7 +186,7 @@ internal static class PeWriter | |||
| debugDirectoryBuilder = null; | |||
| } | |||
|
|
|||
| var peBuilder = new ManagedPEBuilder( | |||
| var peBuilder = new ExtendedPEBuilder( | |||
There was a problem hiding this comment.
ExtendedPEBuilder [](start = 32, length = 17)
Why not have a separate ReferenceAssemblyBuilder and only create it when emitting ref assembly? Instead of heaving a more generic builder whose behavior differs based on a boolean #Resolved
There was a problem hiding this comment.
Tried that approach, but it made logic below more complicated (ExtendedPeBuilder has an overload of Serialize with an extra parameter). #Resolved
| buffer[read++] = (char)current; | ||
| } | ||
|
|
||
| return new string(buffer, 0, read); |
There was a problem hiding this comment.
new string(buffer, 0, read); [](start = 18, length = 29)
Encoding.UTF8.GetBytes(bytes, 0, read) #Resolved
|
|
||
| string ReadSectionName() | ||
| { | ||
| int length = 8; |
There was a problem hiding this comment.
length [](start = 16, length = 6)
const int maxLength #Resolved
| for (int i = 0; i < count; i++) | ||
| { | ||
| // Section: Name (8) | ||
| string name = ReadSectionName(); |
There was a problem hiding this comment.
string name = ReadSectionName(); [](start = 16, length = 32)
Seems like this could be simplified quite a bit.
Something like
bytes[] name = ReadBytes(8);
if (name.Length == 8 && name[0] = '.' && name[1] == 'm' && name[2] == 'v' ...)
{
read data
}
else
{
Skip(size_of_section_header - 8);
}
#Resolved
|
@CyrusNajmabadi @heejaechang Note that the MVID is still always emitted in its regular spot. Also, the new section is currently only added for ref assemblies (emit metadata-only and strip private members), so I don't expect that would affect OOP. #Resolved |
|
@KirillOsenkov FYI, one more way to get an MVID (just for ref assemblies). #Resolved |
|
This is cool. CC @jbevain - how does this feel? Can you think of a better/simpler place where to hide an MVID in the format? #Resolved |
|
@jbevain @KirillOsenkov Just for context, there is another option that we brainstormed, but eventually rejected: part of the compilation hash gets stored in the date/timestamp. |
|
@tmat I think I addressed your feedback. Let me know if good to go. Thanks #Resolved |
| // Section: Name (8) | ||
| byte[] name = ReadBytes(8); | ||
| 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.
name[4] == 'd' [](start = 74, length = 14)
&& name[5] == '\0' so that we don't match on ".mvidXXX" #Resolved
| return new Guid(guidBytes); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
Nit: no need for else since the if-statement returns #Resolved
|
|
|
Thanks @tmat @dotnet/roslyn-compiler for a second review. |
docs/features/refout.md
Outdated
| - Non-public attributes on public APIs (emit attribute based on accessibility rule) | ||
| - ref assemblies and NoPia | ||
| - `/refout` and `/addmodule`, should we disallow this combination? | ||
| - `/refout` and `/addmodule`, should we disallow this combination? (answer: yes. This will not be supported in C# 7.1) |
There was a problem hiding this comment.
Are these still open questions? If not, consider moving to another section in the document. #Resolved
|
|
||
| if (hasMvidSection) | ||
| { | ||
| Assert.Equal(mvidFromModuleDefinition, mvidFromMvidReader); |
There was a problem hiding this comment.
Are we checking the mvids are non-default? #Resolved
| /// </summary> | ||
| private void VerifyMvid(MemoryStream stream, bool hasMvidSection) | ||
| { | ||
| Guid mvidFromModuleDefinition; |
There was a problem hiding this comment.
Declaration can be moved into using, to match mvidFromMvidReader. Alternatively, mvidFromMvidReader and compare could be moved outside the using. #Resolved
|
|
||
| BaseStream.Position = pointerToRawData; | ||
| byte[] guidBytes = new byte[16]; | ||
| BaseStream.Read(guidBytes, 0, 16); |
There was a problem hiding this comment.
BaseStream.Read(guidBytes, 0, 16); [](start = 20, length = 34)
Not checking how many bytes were read. #Resolved
|
|
||
| builder.Add(new Section(MvidSectionName, SectionCharacteristics.MemRead | | ||
| SectionCharacteristics.ContainsInitializedData | | ||
| SectionCharacteristics.MemDiscardable)); |
There was a problem hiding this comment.
For testing it would be useful to test reading for scenarios when .mvid section isn't first. #Resolved
There was a problem hiding this comment.
@jcouv This might be handy for testing such scenarios: https://source.dot.net/#System.Reflection.Metadata.Tests/PortableExecutable/PEBuilderTests.cs
There was a problem hiding this comment.
@tmat I'm going to bake in the assumption that the .mvid section is the first one. #Resolved
There was a problem hiding this comment.
It'd be better not to. Just in case someone runs another tool that adds a section. That should be perfectly fine thing to do.
There was a problem hiding this comment.
@tmat The test you pointed me to doesn't dispose new PEReader(...) (line 496), as Chuck pointed out when I mirrored the test. You may want to review that file for other instances.
|
Done with review pass. #Closed |
| return FindMvidInSections(sections, reader); | ||
| } | ||
|
|
||
| private static Guid FindMvidInSections(ushort count, BinaryReader reader) |
There was a problem hiding this comment.
count is not used. Should we check there is at least one section? #Resolved
There was a problem hiding this comment.
Good point, thanks #Resolved
| private Blob _mvidSectionFixup = default(Blob); | ||
|
|
||
| // Only include the .mvid section in ref assemblies | ||
| private bool _withMvidSection; |
|
@tmat @AlekseyTs I added a test with .mvid section last. |
|
@dotnet-bot test windows_release_unit64_prtest please |
| peBlob.WriteContentTo(peStream); | ||
|
|
||
| peStream.Position = 0; | ||
| var peReader = new PEReader(peStream); |
There was a problem hiding this comment.
using (var peReader = ...) { ... } #Resolved
| for (int i = 0; i < count; i++) | ||
| { | ||
|
|
||
| // .mvid section must be first, if it's there |
| { | ||
| return new Guid(guidBytes); | ||
| } | ||
| } |
|
LGTM |
| } | ||
|
|
||
| reader.BaseStream.Seek(position, SeekOrigin.Begin); | ||
| return true; |
There was a problem hiding this comment.
return true; [](start = 12, length = 12)
We probably shouldn't assume that Seek succeeds. Consider return reader.BaseStream.Seek(position, SeekOrigin.Begin) == position; instead.
| MoveTo(0x3C, reader); | ||
| uint lfanew = reader.ReadUInt32(); | ||
|
|
||
| MoveTo(lfanew, reader); // jump over the MS DOS Stub to the PE Signature |
There was a problem hiding this comment.
MoveTo(lfanew, reader) [](start = 12, length = 22)
The result is not checked, we do not know where we are moving to, therefore, cannot assume that it is safe to continue after that.
| } | ||
|
|
||
| // DOS Header: Address of PE Signature (at 0x3C) | ||
| MoveTo(0x3C, reader); |
There was a problem hiding this comment.
MoveTo(0x3C, reader); [](start = 12, length = 21)
The result is not checked.
There was a problem hiding this comment.
It was already checked above:
// DOS Header (64), DOS Stub (64), PE Signature (4), COFF Header (20), PE Header (224), 1x Section Header (40)
if (reader.BaseStream.Length < 64 + 64 + 4 + 20 + 224 + 40)
| MoveTo(lfanew, reader); // jump over the MS DOS Stub to the PE Signature | ||
|
|
||
| // PE Signature ('P' 'E' null null) | ||
| if (reader.ReadUInt32() != 0x00004550) |
There was a problem hiding this comment.
if (reader.ReadUInt32() != 0x00004550) [](start = 12, length = 38)
This looks like an attempt to read at an unknown location, which can throw if we are near the end of the stream. #Closed
There was a problem hiding this comment.
Already checked above. #Resolved
| { | ||
| return s_empty; | ||
| } | ||
| if (!ReadUInt32(reader, out uint lfanew)) |
There was a problem hiding this comment.
Not sure what the variable name means. Perhaps 'signatureOffset' instead.
There was a problem hiding this comment.
That's what it's called in the spec, which I agree is not very informative. I'll fix.
|
Thanks all for the reviews, feedback and patience :-) |
|
Relates to #18612 |
The dependency on MetadataReader is causing troubles for MSBuild integration. But the MVID is in a difficult place to extract in the PE and we don't want to replicate much MetadataReader logic.
So, for ref assemblies, we make another copy of the MVID in a new Section (".mvid"). Sections can be extracted much more easily. This PR includes a simple PE reader that finds the MVID in this new location in the binary.
@tmat @dotnet/roslyn-compiler for review.