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 API for reading/writing PDB Checksum Debug Directory entry #24953

Closed
tmat opened this issue Feb 7, 2018 · 11 comments
Closed

Add API for reading/writing PDB Checksum Debug Directory entry #24953

tmat opened this issue Feb 7, 2018 · 11 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Milestone

Comments

@tmat
Copy link
Member

tmat commented Feb 7, 2018

Background

The CodeView debug directory entry in PE/COFF file associates the PE file with one or more PDBs. The CodeView entry and the PDB both store the same PDB ID (for Portable PDB it's 20B for Windows PDB it's 16B of data). Debuggers, symbol servers and other tools use the PDB ID to match the PE file with the PDB.

Although the PDB ID is good enough for finding the right PDB for the PE file it is not good enough for validating that the PDB has not been maliciously modified. PDB Checksum is a new debug directory record that can be used for such validation.

PDB Checksum comprises of crypto hash algorithm name and the hash of the PDB content. See
Specification for details.

Proposed APIs

namespace System.Reflection.PortableExecutable
{
    public enum DebugDirectoryEntryType
    {
        // ...
        PdbChecksum = 19,
        // ...
    }

    public class DebugDirectoryBuilder
    {
        // ...

        // Adds a custom entry - useful for emitting record for which specific API is not available (yet)
        public void AddEntry(DebugDirectoryEntryType type, uint version, uint stamp);

        // Adds a custom entry with data - useful for emitting record for which specific API is not available (yet)
        public void AddEntry<TData>(DebugDirectoryEntryType type, uint version, uint stamp, TData data, Action<BlobBuilder, TData> dataSerializer);

        // Adds PDB Checksum entry
        public void AddPdbChecksumEntry(string algorithmName, ImmutableArray<byte> checksum);

        // ...
   }

    public readonly struct PdbChecksumDebugDirectoryData
    {
        /// <summary>
        /// Checksum algorithm name.
        /// </summary>
        public string AlgorithmName { get; }

        /// <summary>
        /// GUID (Globally Unique Identifier) of the associated PDB.
        /// </summary>
        public ImmutableArray<byte> Checksum { get; }
    }

    public class PEReader
    {
        // ...

        public PdbChecksumDebugDirectoryData ReadPdbChecksumDebugDirectoryData(DebugDirectoryEntry 
entry);
        // ...
    }
}

In addition add the following API to make it easy for the reader of a Portable PDB to determine where the PDB ID is located within the PDB. This information is required when validating PDB checksum.

namespace System.Reflection.Metadata
{
    public sealed class DebugMetadataHeader
    {
        // ...

        /// <summary>
        /// Gets the offset (in bytes) from the start of the metadata blob to the start of the <see cref="Id"/> blob.
        /// </summary>
        public int IdStartOffset { get; }

        // ...
    }
}
@tmat tmat self-assigned this Feb 7, 2018
@tmat
Copy link
Member Author

tmat commented Feb 9, 2018

@nguerrera

@nguerrera
Copy link
Contributor

Looks good.

readonly struct

Can we go back and add this all over the place or is it a breaking change?

@tmat
Copy link
Member Author

tmat commented Feb 9, 2018

@nguerrera I has already been added all over the place :)

@nguerrera
Copy link
Contributor

Oh, yay!

@tmat
Copy link
Member Author

tmat commented Feb 10, 2018

@karelz When is the next API review session that can pick this up?

@karelz
Copy link
Member

karelz commented Feb 10, 2018

@tmat we should be able to review on Tue. Ping Immo just in case (he's back from vacation on Mon). I'll be on vacation starting Tue.

@tmat
Copy link
Member Author

tmat commented Feb 10, 2018

@karelz Cool. Thanks

@terrajobst
Copy link
Member

terrajobst commented Feb 13, 2018

Video

We just took a look. Looks good as proposed. Questions that came up:

  • Should the ImmtuableArray<byte> be a ReadOnlySpan<byte>. We assumed the answer to be no, as S.R.M doesn't depend on span yet and you need to ship downlevel. Once you spannify S.R.M we can still add overloads.
  • IdStartOffset are 32 bit enough? Assumption is yes, but hey :-)

@tmat
Copy link
Member Author

tmat commented Feb 13, 2018

Should the ImmtuableArray be a ReadOnlySpan. We assumed the answer to be no, as S.R.M doesn't depend on span yet and you need to ship downlevel. Once you spannify S.R.M we can still add overloads.

The assumption is correct.

IdStartOffset are 32 bit enough? Assumption is yes, but hey :-)

Yes, ECMA metadata blob can't be larger than 2GB and Portable PDB is an ECMA metadata blob.

@tmat
Copy link
Member Author

tmat commented Feb 13, 2018

@terrajobst @karelz Thanks for review.

@tmat
Copy link
Member Author

tmat commented Feb 28, 2018

Done: dotnet/corefx#26976

@tmat tmat closed this as completed Feb 28, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Projects
None yet
Development

No branches or pull requests

5 participants