Include PDB Checksum in Debug Directory #24711

tmat · 2018-02-08T19:17:43Z

Changes

Generate Portable PDB ID deterministically based on the hash of the content, rather than from a timestamp, regardless of whether /deterministic is set or not.
Store the full secure hash of the PDB content in a new Debug Directory entry. This entry is emitted by default unless emitting a script.

Details

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.

The implementation uses a bit of reflection as a workaround for APIs not yet implemented in System.Reflection.Metadata: https://github.com/dotnet/corefx/issues/26935. This workaround will be removed once we move to the latest SRM. Tracked by #24712.

Public API additions

Adds EmitOptions.PdbChecksumAlgorithm, EmitOptions.WithPdbChecksumAlgorithm and new EmitOptions constructor taking a hash algorithm.

Bugs this fixes

Workarounds, if any

None.

Risk

Small.

Performance impact

Small.

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

tmat · 2018-02-09T02:55:56Z

@dotnet/compiler-api
@dotnet/roslyn-compiler
@ivanbasov @jinujoseph

Please review.

gafter · 2018-02-09T16:37:21Z

Can you please elaborate on the "Risk" identified? How many (and what) tools are likely to be affected?

gafter · 2018-02-09T16:46:37Z

src/Compilers/Core/Portable/Emit/EmitOptions.cs

+                  runtimeMetadataVersion,
+                  tolerateErrors,
+                  includePrivateMembers,
+                  instrumentationKinds: ImmutableArray<InstrumentationKind>.Empty,


instrumentationKinds [](start = 18, length = 20)

I think the value of this parameter should not be Empty, but rather the value of the incoming parameter.

You are right. Apparently we are missing code coverage here.

gafter · 2018-02-09T16:48:57Z

src/Compilers/Core/Portable/Emit/EmitOptions.cs

+            {
+                IncrementalHash.CreateHash(HashAlgorithm).Dispose();
+            }
+            catch


catch [](start = 12, length = 5)

A catch-all like this is a bit scary.

We are calling to external library (BCL). Any exception that happens means the hash algorithm is bad or there is a bug in BCL. Either way we report a diagnostic. Sure, we can hit OOM, or stack overflow, but such exceptions will be rethrown anyways even if we catch them here.

Maybe add this explanation as a comment

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

tmat · 2018-02-09T18:23:49Z

@gafter @MattGertz @jinujoseph

I hear some compat and perf concerns with switching to SHA256 by default. I think I can leave the default source hash be SHA1 and create another PR that switches it to SHA256, so to not block the PDB Checksum feature on this decision. I thought the SHA256 switch is a prereq but on second thought it doesn't need to be.

gafter

Looks good to me other than the issue in the EmitOptions ctor. I realize that this was a preexisting problem, but can you please fix it?

tmat · 2018-02-09T18:25:11Z

@gafter Yes, I'll address the EmitOptions bug.

gafter · 2018-02-09T18:27:13Z

I think splitting the PDB checksum feature from the change in default algorithm (i.e. have two PRs) is a good idea.

MattGertz · 2018-02-09T19:02:00Z

@tmat That seems like a good beginning and we can debate the defaults later on if/as needed.

MattGertz · 2018-02-09T19:03:24Z

@jac009 only as FYI since he was curious about the direction here.

ivanbasov · 2018-02-09T19:39:45Z

src/Compilers/Core/Portable/PEWriter/DebugDirectoryExtensions.cs

+        public string AlgorithmName { get; }
+
+        /// <summary>
+        /// GUID (Globally Unique Identifier) of the associated PDB.


GUID (Globally Unique Identifier) of the associated PDB. [](start = 12, length = 56)

Maybe improve the comment. I think it is a checksum and we could expect it to be unique but it maybe uncommon to refer it as GUID.

Yes, the comment is wrong.

ivanbasov

tmat · 2018-02-09T22:44:09Z

@dotnet/compiler-api
@dotnet/roslyn-compiler
@ivanbasov @jinujoseph

Updated the PR to only add PDB Checksum and leave usage of SHA1 as is (except for hashing PDBs).

tmat · 2018-02-10T01:20:40Z

Test failure: #24758

tmat · 2018-02-10T01:20:47Z

test windows_release_unit64_prtest please

tmat · 2018-02-13T19:20:20Z

@dotnet/roslyn-compiler Can I have a second review please?

tmat · 2018-02-13T23:58:50Z

The change to SHA256 is now in a separate PR: #24820

jaredpar · 2018-02-19T16:23:59Z

src/Compilers/Core/CodeAnalysisTest/InternalUtilities/EnumerableExtensionsTests.cs

 using Xunit;

 namespace Roslyn.Utilities.UnitTests.InternalUtilities
 {
+    using System.Linq;


Please move the using outside the namespace.

Will fix. Somehow the "Add using" added it here :(

Actually, this using is required inside the namespace. Both Roslyn.Utilities and System.Linq namespaces have EnumerableExtensions. We want to test the latter here.

jaredpar · 2018-02-19T16:25:50Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

@@ -1309,7 +1310,9 @@ public new CSharpCommandLineArguments Parse(IEnumerable<string> args, string bas
                fileAlignment: fileAlignment,
                subsystemVersion: subsystemVersion,
                runtimeMetadataVersion: runtimeMetadataVersion,
-                instrumentationKinds: instrumentationKinds.ToImmutableAndFree()
+                instrumentationKinds: instrumentationKinds.ToImmutableAndFree(),
+                // TODO: set from /checksumalgorithm (see https://github.com/dotnet/roslyn/issues/24735)


Why are we holding off on this part, seems important here?

It is not that important. The algorithm used for PDB checksum in this PR is hardcoded to SHA256, which is good enough. Allowing to set the value to SHA384 or SHA512 will be important in future, but it can wait a bit.

The reason to hold off is the default value of this switch. Right now it's SHA1, which is not acceptable for PDB checksum. The default has to be at least SHA256. If we want to use the same switch for both source checksums and PDB checksums then we need to set the default for source checksums to SHA256 first.

* features/compiler: (118 commits) Fix up test failures Post merge cleanup SubstituteNoPiaLocalType - use comparison of SpecialTypes to match bases across distinct core libraries. (dotnet#24965) Respond to PR feedback PR feedback PR Feedback Fix tests Standardized on the doc mcomments Unified most helpers now Standardize the IL helpers Unify the WinRT overloads Unify the CompileAndVerify Unify overloads Fix a name Implement improved overload candidates, aka "Bestest Betterness". (dotnet#24756) Include PDB Checksum in Debug Directory (dotnet#24711) Disable multicore jitting More renames Fixed up the unit tests Fix a compilation issue ...

tmat requested a review from a team as a code owner February 8, 2018 19:17

tmat changed the title ~~PDB Checksum~~ WIP: PDB Checksum Feb 8, 2018

tmat force-pushed the PdbChecksum branch from 19dcb7e to d1abbfe Compare February 8, 2018 20:41

tmat changed the title ~~WIP: PDB Checksum~~ WIP: Use SHA256 instead of SHA1 and emit PDB Checksum Debug Directory entry Feb 9, 2018

tmat changed the title ~~WIP: Use SHA256 instead of SHA1 and emit PDB Checksum Debug Directory entry~~ WIP: Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory Feb 9, 2018

tmat force-pushed the PdbChecksum branch from a885951 to 762411e Compare February 9, 2018 02:43

tmat requested review from a team as code owners February 9, 2018 02:43

tmat force-pushed the PdbChecksum branch from 762411e to b835089 Compare February 9, 2018 02:50

tmat changed the title ~~WIP: Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory~~ Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory Feb 9, 2018

gafter reviewed Feb 9, 2018

View reviewed changes

tmat changed the title ~~Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory~~ WIP: Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory Feb 9, 2018

gafter approved these changes Feb 9, 2018

View reviewed changes

tmat changed the title ~~WIP: Use SHA256 instead of SHA1 in emit and include PDB Checksum in Debug Directory~~ WIP: Include PDB Checksum in Debug Directory Feb 9, 2018

ivanbasov reviewed Feb 9, 2018

View reviewed changes

ivanbasov approved these changes Feb 9, 2018

View reviewed changes

tmat added 2 commits February 9, 2018 14:27

Include PDB Checksum in Debug Directory

181d6de

Xlf files

bb04fea

tmat force-pushed the PdbChecksum branch from b835089 to bb04fea Compare February 9, 2018 22:28

tmat changed the title ~~WIP: Include PDB Checksum in Debug Directory~~ Include PDB Checksum in Debug Directory Feb 9, 2018

tmat added 2 commits February 9, 2018 15:30

Add more validation

ef36b0f

Fix tests

ed03c66

tmat added 2 commits February 9, 2018 17:23

Fix xplat test

32f0d6a

Remove data signature

5cc8902

jaredpar reviewed Feb 19, 2018

View reviewed changes

jaredpar approved these changes Feb 20, 2018

View reviewed changes

tmat merged commit 32a11c4 into dotnet:dev15.7.x Feb 20, 2018

tmat deleted the PdbChecksum branch February 20, 2018 17:38

KevinRansom mentioned this pull request Aug 10, 2018

Include crypto hash of Portable PDB content in the PE debug directory as a new record dotnet/fsharp#4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include PDB Checksum in Debug Directory #24711

Include PDB Checksum in Debug Directory #24711

tmat commented Feb 8, 2018 •

edited

Loading

tmat commented Feb 9, 2018

gafter commented Feb 9, 2018

gafter Feb 9, 2018

tmat Feb 9, 2018

gafter Feb 9, 2018

tmat Feb 9, 2018

ivanbasov Feb 9, 2018

tmat commented Feb 9, 2018

gafter left a comment

tmat commented Feb 9, 2018

gafter commented Feb 9, 2018

MattGertz commented Feb 9, 2018

MattGertz commented Feb 9, 2018

ivanbasov Feb 9, 2018

tmat Feb 9, 2018

ivanbasov left a comment

tmat commented Feb 9, 2018

tmat commented Feb 10, 2018

tmat commented Feb 10, 2018

tmat commented Feb 13, 2018

tmat commented Feb 13, 2018

jaredpar Feb 19, 2018

tmat Feb 20, 2018

tmat Feb 20, 2018

jaredpar Feb 19, 2018

tmat Feb 20, 2018 •

edited

Loading

Include PDB Checksum in Debug Directory #24711

Include PDB Checksum in Debug Directory #24711

Conversation

tmat commented Feb 8, 2018 • edited Loading

Changes

Details

Public API additions

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

tmat commented Feb 9, 2018

gafter commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Feb 9, 2018

gafter left a comment

Choose a reason for hiding this comment

tmat commented Feb 9, 2018

gafter commented Feb 9, 2018

MattGertz commented Feb 9, 2018

MattGertz commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanbasov left a comment

Choose a reason for hiding this comment

tmat commented Feb 9, 2018

tmat commented Feb 10, 2018

tmat commented Feb 10, 2018

tmat commented Feb 13, 2018

tmat commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

tmat commented Feb 8, 2018 •

edited

Loading

tmat Feb 20, 2018 •

edited

Loading