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.

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

Add .mvid section to PE and remove dependency on MetadataReader #19133

Merged
merged 9 commits into from
May 2, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CSharpCompilerEmitTest.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|AnyCPU'" />
<ItemGroup>
<Compile Include="..\..\..\Core\MSBuildTask\MvidReader.cs">
<Link>Emit\MvidReader.cs</Link>
</Compile>
<Compile Include="Attributes\AttributeTests.cs" />
<Compile Include="Attributes\AttributeTests_Assembly.cs" />
<Compile Include="Attributes\AttributeTests_CallerInfoAttributes.cs" />
Expand Down
32 changes: 32 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,11 @@ internal static void Main()

VerifyEntryPoint(output, expectZero: false);
VerifyMethods(output, new[] { "void C.Main()", "C..ctor()" });
VerifyMVID(output, hasMvidSection: false);

VerifyEntryPoint(metadataOutput, expectZero: true);
VerifyMethods(metadataOutput, new[] { "C..ctor()" });
VerifyMVID(metadataOutput, hasMvidSection: true);
Copy link
Member Author

@jcouv jcouv Apr 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

I'd also try to run peverify on the ref assembly. #Resolved

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

Also add a ref assembly test to deterministic tests. #Resolved

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

@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

}

void VerifyEntryPoint(MemoryStream stream, bool expectZero)
Expand All @@ -275,6 +277,34 @@ void VerifyEntryPoint(MemoryStream stream, bool expectZero)
}
}

/// <summary>
/// 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)
Copy link
Member

@jaredpar jaredpar May 1, 2017

Choose a reason for hiding this comment

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

Please use explicit accessibility modifier. #Resolved

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

nit: VerifyMvid instead of VerifyMVID #Resolved

{
Guid mvidFromModuleDefinition;
Copy link
Member

@cston cston May 1, 2017

Choose a reason for hiding this comment

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

Declaration can be moved into using, to match mvidFromMvidReader. Alternatively, mvidFromMvidReader and compare could be moved outside the using. #Resolved

stream.Position = 0;
using (var reader = new PEReader(stream))
{
var metadataReader = reader.GetMetadataReader();
mvidFromModuleDefinition = metadataReader.GetGuid(metadataReader.GetModuleDefinition().Mvid);

stream.Position = 0;
var mvidFromMvidReader = BuildTasks.MvidReader.ReadAssemblyMvid(stream);

if (hasMvidSection)
{
Assert.Equal(mvidFromModuleDefinition, mvidFromMvidReader);
Copy link
Member

@cston cston May 1, 2017

Choose a reason for hiding this comment

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

Are we checking the mvids are non-default? #Resolved

}
else
{
Assert.NotEqual(Guid.Empty, mvidFromModuleDefinition);
Assert.Equal(Guid.Empty, mvidFromMvidReader);
}
}
}

[Fact]
public void EmitRefAssembly_PrivatePropertySetter()
{
Expand All @@ -296,6 +326,8 @@ public class C
VerifyMethods(output, new[] { "System.Int32 C.<PrivateSetter>k__BackingField", "System.Int32 C.PrivateSetter.get", "void C.PrivateSetter.set",
"C..ctor()", "System.Int32 C.PrivateSetter { get; private set; }" });
VerifyMethods(metadataOutput, new[] { "System.Int32 C.PrivateSetter.get", "C..ctor()", "System.Int32 C.PrivateSetter { get; }" });
VerifyMVID(output, hasMvidSection: false);
VerifyMVID(metadataOutput, hasMvidSection: true);
}
}

Expand Down
6 changes: 1 addition & 5 deletions src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

using System;
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.PortableExecutable;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;

Expand Down Expand Up @@ -85,10 +83,8 @@ private bool Copy()
private Guid ExtractMvid(string path)
{
using (FileStream source = File.OpenRead(path))
using (var reader = new PEReader(source))
{
var metadataReader = reader.GetMetadataReader();
return metadataReader.GetGuid(metadataReader.GetModuleDefinition().Mvid);
return MvidReader.ReadAssemblyMvid(source);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/Core/MSBuildTask/MSBuildTask.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
</Compile>
<Compile Include="AssemblyResolution.cs" />
<Compile Include="CanonicalError.cs" />
<Compile Include="MvidReader.cs" />
<Compile Include="CopyRefAssembly.cs" />
<Compile Include="ValidateBootstrap.cs" />
<Compile Include="CommandLineBuilderExtension.cs" />
Expand Down
143 changes: 143 additions & 0 deletions src/Compilers/Core/MSBuildTask/MvidReader.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// 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;

namespace Microsoft.CodeAnalysis.BuildTasks
{
public sealed class MvidReader : BinaryReader
Copy link
Member

Choose a reason for hiding this comment

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

Are there callers that instantiate this type? If not, could this be a static class (with a single public method) that operates on a BinaryReader directly?

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

I'll make the constructor private.
I gave your suggestion a shot (making the class static, but it made the code worse). For instance, I need to add ReadUInt32 and ReadUInt16 wrapper methods, and then pass a BinaryReader to pretty much every line in this class. #Resolved

{
public MvidReader(Stream stream) : base(stream)
{
}

public static Guid ReadAssemblyMvid(Stream stream)
{
try
{
var mvidReader = new MvidReader(stream);
return mvidReader.FindMvid();
}
catch (EndOfStreamException)
{
}

return Guid.Empty;
Copy link
Member

@jaredpar jaredpar May 1, 2017

Choose a reason for hiding this comment

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

Feel like this is essentially hiding failure here. Consider using a TryReadAssemblyMvid pattern and potentially have a wrapper named ReadAssemblyMvidOrEmpty #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

@jaredpar I renamed the methods. Let me know if that's better.


In reply to: 114092229 [](ancestors = 114092229)

Copy link
Member

Choose a reason for hiding this comment

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

Is the exception handler still needed?

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

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 FindMvid()
{
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());
Copy link
Contributor

@AlekseyTs AlekseyTs May 1, 2017

Choose a reason for hiding this comment

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

MoveTo(ReadUInt32()); [](start = 12, length = 21)

It looks like we are moving to an unknown position without verifying if the destination makes sense. #Resolved

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

Yes. That's why this logic is wrapped in a try/catch (EndOfStreamException). #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs May 1, 2017

Choose a reason for hiding this comment

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

Yes. That's why this logic is wrapped in a try/catch (EndOfStreamException).

But this method is public and it doesn't catch any exceptions. #Resolved

Copy link
Member

@KirillOsenkov KirillOsenkov May 1, 2017

Choose a reason for hiding this comment

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

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);
}

string ReadSectionName()
Copy link
Member

@jaredpar jaredpar May 1, 2017

Choose a reason for hiding this comment

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

Please use explicit accessibility modifier. #Resolved

{
int length = 8;
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

length [](start = 16, length = 6)

const int maxLength #Resolved

int read = 0;
var buffer = new char[length];
var bytes = ReadBytes(length);
while (read < length)
{
var current = bytes[read];
if (current == 0)
break;

buffer[read++] = (char)current;
}

return new string(buffer, 0, read);
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

new string(buffer, 0, read); [](start = 18, length = 29)

Encoding.UTF8.GetBytes(bytes, 0, read) #Resolved

}

Guid FindMvidInSections(ushort count)
Copy link
Member

@jaredpar jaredpar May 1, 2017

Choose a reason for hiding this comment

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

Please use explicit accessibility modifier. #Resolved

{
for (int i = 0; i < count; i++)
{
// Section: Name (8)
string name = ReadSectionName();
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

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


// Section: VirtualSize (4)
uint virtualSize = ReadUInt32();

// Section: VirtualAddress (4), SizeOfRawData (4)
Copy link
Member Author

@jcouv jcouv Apr 30, 2017

Choose a reason for hiding this comment

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

@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

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

Sadly, the PE spec requires the value of FileAlignment to be

... a power of 2 between 512 and 64K, inclusive. #Resolved

Skip(8);

// Section: PointerToRawData (4)
uint pointerToRawData = ReadUInt32();

// Section: PointerToRelocations (4), PointerToLineNumbers (4), NumberOfRelocations (2),
// NumberOfLineNumbers (2), Characteristics (4)
Skip(16);

if (name.Equals(".mvid", StringComparison.Ordinal))
{
// The .mvid section only stores a Guid
Debug.Assert(virtualSize == 16);
Copy link
Contributor

@AlekseyTs AlekseyTs May 1, 2017

Choose a reason for hiding this comment

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

Debug.Assert(virtualSize == 16); [](start = 20, length = 32)

I think we should verify the size, rather than simply assert it. #Resolved


BaseStream.Position = pointerToRawData;
Copy link
Contributor

@AlekseyTs AlekseyTs May 1, 2017

Choose a reason for hiding this comment

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

BaseStream.Position = pointerToRawData; [](start = 20, length = 39)

Should we use Move() here? #Resolved

byte[] guidBytes = new byte[16];
BaseStream.Read(guidBytes, 0, 16);
Copy link
Contributor

@AlekseyTs AlekseyTs May 1, 2017

Choose a reason for hiding this comment

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

BaseStream.Read(guidBytes, 0, 16); [](start = 20, length = 34)

Not checking how many bytes were read. #Resolved


return new Guid(guidBytes);
}
}

return Guid.Empty;
}

public void Skip(int bytes)
{
BaseStream.Seek(bytes, SeekOrigin.Current);
}

public void MoveTo(uint position)
{
BaseStream.Seek(position, SeekOrigin.Begin);
}
Copy link
Member

Choose a reason for hiding this comment

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

These methods can be private.

}
}
1 change: 0 additions & 1 deletion src/Compilers/Core/MSBuildTask/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"System.IO.Pipes": "4.3.0",
"System.Linq": "4.3.0",
"System.Reflection": "4.3.0",
"System.Reflection.Metadata": "1.4.2",
"System.Security.AccessControl": "4.3.0",
"System.Security.Cryptography.Algorithms": "4.3.0",
"System.Security.Principal.Windows": "4.3.0",
Expand Down
106 changes: 100 additions & 6 deletions src/Compilers/Core/Portable/PEWriter/PeWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ internal static bool WritePeToStream(
MethodDefinitionHandle entryPointHandle;
MethodDefinitionHandle debugEntryPointHandle;
mdWriter.GetEntryPoints(out entryPointHandle, out debugEntryPointHandle);

if (!debugEntryPointHandle.IsNil)
{
nativePdbWriterOpt?.SetEntryPoint((uint)MetadataTokens.GetToken(debugEntryPointHandle));
Expand Down Expand Up @@ -186,7 +186,7 @@ internal static bool WritePeToStream(
debugDirectoryBuilder = null;
}

var peBuilder = new ManagedPEBuilder(
var peBuilder = new ExtendedPEBuilder(
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

Tried that approach, but it made logic below more complicated (ExtendedPeBuilder has an overload of Serialize with an extra parameter). #Resolved

peHeaderBuilder,
metadataRootBuilder,
ilBuilder,
Expand All @@ -197,12 +197,13 @@ internal static bool WritePeToStream(
CalculateStrongNameSignatureSize(context.Module),
entryPointHandle,
properties.CorFlags,
deterministicIdProvider);
deterministicIdProvider,
metadataOnly && !context.IncludePrivateMembers);

var peBlob = new BlobBuilder();
var peContentId = peBuilder.Serialize(peBlob);
var peContentId = peBuilder.Serialize(peBlob, out Blob mvidSectionFixup);

PatchModuleVersionIds(mvidFixup, mvidStringFixup, peContentId.Guid);
PatchModuleVersionIds(mvidFixup, mvidSectionFixup, mvidStringFixup, peContentId.Guid);

try
{
Expand All @@ -216,7 +217,7 @@ internal static bool WritePeToStream(
return true;
}

private static void PatchModuleVersionIds(Blob guidFixup, Blob stringFixup, Guid mvid)
private static void PatchModuleVersionIds(Blob guidFixup, Blob guidSectionFixup, Blob stringFixup, Guid mvid)
{
if (!guidFixup.IsDefault)
{
Expand All @@ -225,6 +226,13 @@ private static void PatchModuleVersionIds(Blob guidFixup, Blob stringFixup, Guid
Debug.Assert(writer.RemainingBytes == 0);
}

if (!guidSectionFixup.IsDefault)
{
var writer = new BlobWriter(guidSectionFixup);
writer.WriteGuid(mvid);
Debug.Assert(writer.RemainingBytes == 0);
}

if (!stringFixup.IsDefault)
{
var writer = new BlobWriter(stringFixup);
Expand Down Expand Up @@ -322,5 +330,91 @@ private static int CalculateStrongNameSignatureSize(CommonPEModuleBuilder module

return (keySize < 128 + 32) ? 128 : keySize - 32;
}

/// <summary>
/// This PEBuilder adds an .mvid section.
/// </summary>
private class ExtendedPEBuilder : ManagedPEBuilder
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

cl [](start = 16, length = 2)

sealed #Resolved

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

ExtendedPEBuilder [](start = 22, length = 17)

Move to a separate file. #Resolved

{
private const string MvidSectionName = ".mvid";
public const int SizeOfGuid = 16;

// When the section is built with a placeholder, the placeholder blob is saved for later fixing up.
private Blob _mvidSectionFixup = default(Blob);

// Only include the .mvid section in ref assemblies
private bool _withMvidSection;

public ExtendedPEBuilder(
PEHeaderBuilder header,
MetadataRootBuilder metadataRootBuilder,
BlobBuilder ilStream,
BlobBuilder mappedFieldData,
BlobBuilder managedResources,
ResourceSectionBuilder nativeResources,
DebugDirectoryBuilder debugDirectoryBuilder,
int strongNameSignatureSize,
MethodDefinitionHandle entryPoint,
CorFlags flags,
Func<IEnumerable<Blob>, BlobContentId> deterministicIdProvider,
bool withMvidSection)
: base(header, metadataRootBuilder, ilStream, mappedFieldData, managedResources, nativeResources,
debugDirectoryBuilder, strongNameSignatureSize, entryPoint, flags, deterministicIdProvider)
{
_withMvidSection = withMvidSection;
}

protected override ImmutableArray<Section> CreateSections()
{
var baseSections = base.CreateSections();

if (_withMvidSection)
{
var builder = ArrayBuilder<Section>.GetInstance(baseSections.Length + 1);

builder.Add(new Section(MvidSectionName, SectionCharacteristics.MemRead |
SectionCharacteristics.ContainsInitializedData |
SectionCharacteristics.MemDiscardable));

builder.AddRange(baseSections);
return builder.ToImmutableAndFree();
}
else
{
return baseSections;
}
}

protected override BlobBuilder SerializeSection(string name, SectionLocation location)
{
if (name.Equals(MvidSectionName, StringComparison.Ordinal))
{
Debug.Assert(_withMvidSection);
return SerializeMvidSection(location);
}

return base.SerializeSection(name, location);
}

internal BlobContentId Serialize(BlobBuilder peBlob, out Blob mvidSectionFixup)
{
var result = base.Serialize(peBlob);
mvidSectionFixup = _mvidSectionFixup;
return result;
}

private BlobBuilder SerializeMvidSection(SectionLocation location)
{
var sectionBuilder = new BlobBuilder();

// The guid will be filled in later:
_mvidSectionFixup = sectionBuilder.ReserveBytes(SizeOfGuid);
var mvidWriter = new BlobWriter(_mvidSectionFixup);
mvidWriter.WriteBytes(0, _mvidSectionFixup.Length);
Debug.Assert(mvidWriter.RemainingBytes == 0);

return sectionBuilder;
}
}
}
}