From 7dfbebcef67e00a0bfcb16bbdafbbf4b7e96c514 Mon Sep 17 00:00:00 2001 From: Peter Shrosbree Date: Sun, 4 Oct 2026 21:42:06 -0700 Subject: [PATCH] Fix ilasm PdbChecksum and the PDB ID under -DET ilasm computed the PdbChecksum debug directory entry over the #Pdb stream only, while PE-COFF.md requires the hash of the entire PDB file with its 20-byte PDB ID zeroed. Under -DET it also wrote only the deterministic GUID into the #Pdb stream, so the PDB ID's timestamp stayed 0 while the CodeView entry carried the deterministic stamp, and PEReader.TryOpenAssociatedPortablePdb rejected the pair. Compute the checksum by serializing the PDB with its ID temporarily zeroed, and write both the GUID and the timestamp into the #Pdb stream. Extend TestPortablePdbDebugDirectory to compare all 20 bytes of the PDB ID with the CodeView entry and to verify the PdbChecksum entry, with and without -DET. --- src/coreclr/ilasm/portable_pdb.cpp | 29 ++++++++++-- src/coreclr/ilasm/portable_pdb.h | 4 +- src/coreclr/ilasm/writer.cpp | 10 ++-- src/coreclr/md/compiler/emit.cpp | 23 ++-------- src/coreclr/md/compiler/regmeta.h | 9 ++-- src/coreclr/md/enc/pdbheap.cpp | 20 ++++---- src/coreclr/md/inc/pdbheap.h | 3 +- src/coreclr/md/inc/portablepdbmdi.h | 9 ++-- .../PortablePdb/IlasmPortablePdbTester.cs | 46 ++++++++++++------- .../IlasmPortablePdbTesterCommon.cs | 4 +- 10 files changed, 86 insertions(+), 71 deletions(-) diff --git a/src/coreclr/ilasm/portable_pdb.cpp b/src/coreclr/ilasm/portable_pdb.cpp index 3aa26fe1597a5f..7fa5b1fb2e92f4 100644 --- a/src/coreclr/ilasm/portable_pdb.cpp +++ b/src/coreclr/ilasm/portable_pdb.cpp @@ -160,15 +160,36 @@ HRESULT PortablePdbWriter::BuildPdbStream(IMetaDataEmit3* peEmitter, mdMethodDef return hr; } -HRESULT PortablePdbWriter::ComputeSha256PdbStreamChecksum(BYTE(&checksum)[32]) +HRESULT PortablePdbWriter::ComputeSha256PdbChecksum(BYTE(&checksum)[32]) { - return m_ilasmPdbWriter->ComputeSha256PdbStreamChecksum(Sha256Hash, checksum); + // The checksum is the hash of the entire PDB file with its 20-byte PDB ID zeroed + // (see "PDB Checksum Debug Directory Entry" in docs/design/specs/PE-COFF.md). + // Serialize the PDB the same way it is saved later, with the ID temporarily zeroed, + // then restore the ID. + HRESULT hr = S_OK; + HRESULT restoreHr = S_OK; + DWORD pdbSize = 0; + BYTE* pdbData = NULL; + + if (FAILED(hr = m_ilasmPdbWriter->ChangePdbStreamId(GUID(), 0))) goto exit; + if (FAILED(hr = m_pdbEmitter->GetSaveSize(cssAccurate, &pdbSize))) goto exit; + pdbData = new BYTE[pdbSize]; + if (FAILED(hr = m_pdbEmitter->SaveToMemory(pdbData, pdbSize))) goto exit; + hr = Sha256Hash(pdbData, pdbSize, checksum, sizeof(checksum)); + +exit: + restoreHr = m_ilasmPdbWriter->ChangePdbStreamId(m_pdbStream.id.pdbGuid, m_pdbStream.id.pdbTimeStamp); + if (SUCCEEDED(hr)) + hr = restoreHr; + delete[] pdbData; + return hr; } -HRESULT PortablePdbWriter::ChangePdbStreamGuid(REFGUID newGuid) +HRESULT PortablePdbWriter::ChangePdbStreamId(REFGUID newGuid, const ULONG newTimestamp) { m_pdbStream.id.pdbGuid = newGuid; - return m_ilasmPdbWriter->ChangePdbStreamGuid(newGuid); + m_pdbStream.id.pdbTimeStamp = newTimestamp; + return m_ilasmPdbWriter->ChangePdbStreamId(newGuid, newTimestamp); } HRESULT PortablePdbWriter::DefineDocument(char* name, GUID* language) diff --git a/src/coreclr/ilasm/portable_pdb.h b/src/coreclr/ilasm/portable_pdb.h index 5fd69d0fbb6034..00a9c0fc8c0efb 100644 --- a/src/coreclr/ilasm/portable_pdb.h +++ b/src/coreclr/ilasm/portable_pdb.h @@ -50,8 +50,8 @@ class PortablePdbWriter void SetTimestamp(const ULONG newTimestamp); Document* GetCurrentDocument(); HRESULT BuildPdbStream(IMetaDataEmit3* peEmitter, mdMethodDef entryPoint); - HRESULT ComputeSha256PdbStreamChecksum(BYTE (&checksum)[32]); - HRESULT ChangePdbStreamGuid(REFGUID newGuid); + HRESULT ComputeSha256PdbChecksum(BYTE (&checksum)[32]); + HRESULT ChangePdbStreamId(REFGUID newGuid, const ULONG newTimestamp); HRESULT DefineDocument(char* name, GUID* language); HRESULT DefineSequencePoints(Method* method); HRESULT DefineLocalScope(Method* method); diff --git a/src/coreclr/ilasm/writer.cpp b/src/coreclr/ilasm/writer.cpp index a985d2ec196c63..7076922965b8ba 100644 --- a/src/coreclr/ilasm/writer.cpp +++ b/src/coreclr/ilasm/writer.cpp @@ -79,8 +79,8 @@ HRESULT Assembler::InitMetaData() if (m_fDeterministic) { - // When build determinism is enabled, the PDB checksum is computed with these fields set to zero. - // The GUID and timestamp will be updated after. + // In deterministic mode, the random GUID and timestamp set by Init() must not reach the output. + // The deterministic PDB ID is written in CreatePEFile, once the metadata hash is known. m_pPortablePdbWriter->SetGuid(GUID()); m_pPortablePdbWriter->SetTimestamp(0); } @@ -1480,16 +1480,14 @@ HRESULT Assembler::CreatePEFile(_In_ __nullterminated WCHAR *pwzOutputFilename) if (FAILED(hr = m_pPortablePdbWriter->BuildPdbStream(m_pEmitter, entryPoint))) goto exit; BYTE pdbChecksum[32]; - if (FAILED(hr = m_pPortablePdbWriter->ComputeSha256PdbStreamChecksum(pdbChecksum))) goto exit; + if (FAILED(hr = m_pPortablePdbWriter->ComputeSha256PdbChecksum(pdbChecksum))) goto exit; if (m_fDeterministic) { // Now that the PDB checksum has been computed, update the GUID and timestamp _ASSERTE(*(m_pPortablePdbWriter->GetGuid()) == GUID()); - if (FAILED(hr = m_pPortablePdbWriter->ChangePdbStreamGuid(deterministicGuid))) goto exit; - _ASSERTE(m_pPortablePdbWriter->GetTimestamp() == 0); - m_pPortablePdbWriter->SetTimestamp(deterministicTimestamp); + if (FAILED(hr = m_pPortablePdbWriter->ChangePdbStreamId(deterministicGuid, deterministicTimestamp))) goto exit; } if (FAILED(hr=CreateDebugDirectory(pdbChecksum))) goto exit; diff --git a/src/coreclr/md/compiler/emit.cpp b/src/coreclr/md/compiler/emit.cpp index 50ff3c1ad2e9d0..7c518799dc5e43 100644 --- a/src/coreclr/md/compiler/emit.cpp +++ b/src/coreclr/md/compiler/emit.cpp @@ -2116,29 +2116,16 @@ STDMETHODIMP RegMeta::DefineLocalVariable( // S_OK or error. } // RegMeta::DefineLocalVariable //******************************************************************************* -// ComputeSha256PdbStreamChecksum +// ChangePdbStreamId //******************************************************************************* -STDMETHODIMP RegMeta::ComputeSha256PdbStreamChecksum( - HRESULT (*computeSha256)(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize), - BYTE (&checksum)[32]) +STDMETHODIMP RegMeta::ChangePdbStreamId( + REFGUID newGuid, + ULONG newTimestamp) { #ifdef FEATURE_METADATA_EMIT_IN_DEBUGGER return E_NOTIMPL; #else //!FEATURE_METADATA_EMIT_IN_DEBUGGER - return m_pStgdb->m_pPdbHeap->ComputeSha256Checksum(computeSha256, checksum); -#endif //!FEATURE_METADATA_EMIT_IN_DEBUGGER -} - -//******************************************************************************* -// ChangePdbStreamGuid -//******************************************************************************* -STDMETHODIMP RegMeta::ChangePdbStreamGuid( - REFGUID newGuid) -{ -#ifdef FEATURE_METADATA_EMIT_IN_DEBUGGER - return E_NOTIMPL; -#else //!FEATURE_METADATA_EMIT_IN_DEBUGGER - return m_pStgdb->m_pPdbHeap->SetDataGuid(newGuid); + return m_pStgdb->m_pPdbHeap->SetDataId(newGuid, newTimestamp); #endif //!FEATURE_METADATA_EMIT_IN_DEBUGGER } diff --git a/src/coreclr/md/compiler/regmeta.h b/src/coreclr/md/compiler/regmeta.h index b16a1326b02502..9a5bcbf3208904 100644 --- a/src/coreclr/md/compiler/regmeta.h +++ b/src/coreclr/md/compiler/regmeta.h @@ -1133,12 +1133,9 @@ class RegMeta : //***************************************************************************** // IILAsmPortablePdbWriter methods //***************************************************************************** - STDMETHODIMP ComputeSha256PdbStreamChecksum( // S_OK or error. - HRESULT (*computeSha256)(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize), // [IN] - BYTE (&checksum)[32]); // [OUT] 256-bit Pdb checksum - - STDMETHODIMP ChangePdbStreamGuid( // S_OK or error. - REFGUID newGuid); // [IN] GUID to use as the PDB GUID + STDMETHODIMP ChangePdbStreamId( // S_OK or error. + REFGUID newGuid, // [IN] GUID to use as the PDB GUID + ULONG newTimestamp); // [IN] Timestamp to use as the PDB stamp #endif // FEATURE_METADATA_EMIT_PORTABLE_PDB //***************************************************************************** diff --git a/src/coreclr/md/enc/pdbheap.cpp b/src/coreclr/md/enc/pdbheap.cpp index d402cddc0a7316..1e406a4d2e5f75 100644 --- a/src/coreclr/md/enc/pdbheap.cpp +++ b/src/coreclr/md/enc/pdbheap.cpp @@ -68,23 +68,25 @@ HRESULT PdbHeap::SetData(PORT_PDB_STREAM* data) __checkReturn -HRESULT PdbHeap::SetDataGuid(REFGUID newGuid) +HRESULT PdbHeap::SetDataId(REFGUID newGuid, ULONG newTimestamp) { _ASSERTE(m_size >= sizeof(PDB_ID)); - if (memcpy_s(m_data, m_size, &newGuid, sizeof(GUID))) + PDB_ID id; + id.pdbGuid = newGuid; + id.pdbTimeStamp = newTimestamp; + +#if BIGENDIAN + SwapGuid(&id.pdbGuid); + id.pdbTimeStamp = VAL32(id.pdbTimeStamp); +#endif + + if (memcpy_s(m_data, m_size, &id, sizeof(id))) return E_FAIL; return S_OK; } -__checkReturn -HRESULT PdbHeap::ComputeSha256Checksum(HRESULT (*computeSha256)(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize), BYTE (&checksum)[32]) -{ - _ASSERTE(m_size >= sizeof(PDB_ID)); - return computeSha256(m_data, m_size, (BYTE*)&checksum, sizeof(checksum)); -} - __checkReturn HRESULT PdbHeap::SaveToStream(IStream* stream) { diff --git a/src/coreclr/md/inc/pdbheap.h b/src/coreclr/md/inc/pdbheap.h index 88bae893f86d7e..1c6387b2ce52cb 100644 --- a/src/coreclr/md/inc/pdbheap.h +++ b/src/coreclr/md/inc/pdbheap.h @@ -21,8 +21,7 @@ class PdbHeap ~PdbHeap(); __checkReturn HRESULT SetData(PORT_PDB_STREAM* data); - __checkReturn HRESULT SetDataGuid(REFGUID newGuid); - __checkReturn HRESULT ComputeSha256Checksum(HRESULT (*computeSha256)(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize), BYTE (&checksum)[32]); + __checkReturn HRESULT SetDataId(REFGUID newGuid, ULONG newTimestamp); __checkReturn HRESULT SaveToStream(IStream* stream); BOOL IsEmpty(); ULONG GetSize(); diff --git a/src/coreclr/md/inc/portablepdbmdi.h b/src/coreclr/md/inc/portablepdbmdi.h index fffdfbe95a5ff9..3a598744f0c4d3 100644 --- a/src/coreclr/md/inc/portablepdbmdi.h +++ b/src/coreclr/md/inc/portablepdbmdi.h @@ -100,12 +100,9 @@ EXTERN_GUID(IID_IILAsmPortablePdbWriter, 0x8b2db1f0, 0x91f5, 0x4c99, 0xbb, 0x07, #define INTERFACE IILAsmPortablePdbWriter DECLARE_INTERFACE_(IILAsmPortablePdbWriter, IUnknown) { - STDMETHOD(ComputeSha256PdbStreamChecksum)( // S_OK or error. - HRESULT (*computeSha256)(BYTE* pSrc, DWORD srcSize, BYTE* pDst, DWORD dstSize), // [IN] - BYTE (&checksum)[32]) PURE; // [OUT] 256-bit Pdb checksum - - STDMETHOD(ChangePdbStreamGuid)( // S_OK or error. - REFGUID newGuid) PURE; // [IN] GUID to use as the PDB GUID + STDMETHOD(ChangePdbStreamId)( // S_OK or error. + REFGUID newGuid, // [IN] GUID to use as the PDB GUID + ULONG newTimestamp) PURE; // [IN] Timestamp to use as the PDB stamp }; #ifdef __cplusplus diff --git a/src/tests/ilasm/PortablePdb/IlasmPortablePdbTester.cs b/src/tests/ilasm/PortablePdb/IlasmPortablePdbTester.cs index e4089f9812e377..85bcaf29feac93 100644 --- a/src/tests/ilasm/PortablePdb/IlasmPortablePdbTester.cs +++ b/src/tests/ilasm/PortablePdb/IlasmPortablePdbTester.cs @@ -5,7 +5,9 @@ using System.Linq; using Xunit; using System.Collections.Generic; +using System.Collections.Immutable; using System.Reflection.Metadata; +using System.Security.Cryptography; namespace IlasmPortablePdbTests { @@ -27,15 +29,17 @@ public IlasmPortablePdbTester() IlasmFile = IlasmFileName + NativeExtension; } - // Tests whether pe file includes portable pdb codeview debug directory - // and its contents against the generated portable pdb metadata file + // Tests whether pe file includes portable pdb codeview and pdb checksum debug directory entries + // and their contents against the generated portable pdb file, with and without deterministic output [Theory] - [InlineData("TestPdbDebugDirectory1.il")] - [InlineData("TestPdbDebugDirectory2.il")] - public void TestPortablePdbDebugDirectory(string ilSource) + [InlineData("TestPdbDebugDirectory1.il", false)] + [InlineData("TestPdbDebugDirectory1.il", true)] + [InlineData("TestPdbDebugDirectory2.il", false)] + [InlineData("TestPdbDebugDirectory2.il", true)] + public void TestPortablePdbDebugDirectory(string ilSource, bool deterministic) { var ilasm = IlasmPortablePdbTesterCommon.GetIlasmFullPath(CoreRootVar, IlasmFile); - IlasmPortablePdbTesterCommon.Assemble(ilasm, ilSource, TestDir, out string dll, out string pdb); + IlasmPortablePdbTesterCommon.Assemble(ilasm, ilSource, TestDir, out string dll, out string pdb, deterministic); using (var peStream = new FileStream(dll, FileMode.Open, FileAccess.Read)) { @@ -51,22 +55,32 @@ public void TestPortablePdbDebugDirectory(string ilSource) Assert.Equal(1, portablePdbDbgEntry.Age); Assert.Equal(pdb, portablePdbDbgEntry.Path); + var pdbChecksumEntry = Assert.Single(dbgDirEntries, entry => entry.Type == DebugDirectoryEntryType.PdbChecksum); + var pdbChecksum = peReader.ReadPdbChecksumDebugDirectoryData(pdbChecksumEntry); + Assert.Equal("SHA256", pdbChecksum.AlgorithmName); + + var pdbImage = File.ReadAllBytes(pdb); + using (var pdbImageReaderProvider = MetadataReaderProvider.FromPortablePdbImage(ImmutableArray.Create(pdbImage))) + { + var pdbHeader = pdbImageReaderProvider.GetMetadataReader().DebugMetadataHeader; + Assert.NotNull(pdbHeader); + + // check pdb id (guid and stamp) against the codeview entry + var pdbId = new BlobContentId(pdbHeader.Id); + Assert.Equal(portablePdbDbgEntry.Guid, pdbId.Guid); + Assert.Equal(dbgEntry.Stamp, pdbId.Stamp); + + // check pdb checksum: the hash of the entire pdb file with its 20-byte pdb id zeroed + Array.Clear(pdbImage, pdbHeader.IdStartOffset, pdbHeader.Id.Length); + Assert.Equal(SHA256.HashData(pdbImage), pdbChecksum.Checksum.ToArray()); + } + using (var pdbReaderProvider = IlasmPortablePdbTesterCommon.GetMetadataReaderProvider(dll, pdb, peReader, false)) { var portablePdbMdReader = pdbReaderProvider.GetMetadataReader(); Assert.NotNull(portablePdbMdReader); // check pdb stream Assert.NotNull(portablePdbMdReader.DebugMetadataHeader); - // check pdb guid - var pdbGuid = portablePdbDbgEntry.Guid.ToByteArray(); - var pdbId = portablePdbMdReader.DebugMetadataHeader.Id.ToArray(); - int i = 0; - foreach (var pdbGuidByte in pdbGuid) - { - Assert.True(i < pdbId.Length); - Assert.Equal(pdbGuidByte, pdbId[i++]); - } - Assert.Equal(i, pdbGuid.Length); var peMdReader = peReader.GetMetadataReader(); Assert.NotNull(peMdReader); diff --git a/src/tests/ilasm/PortablePdb/IlasmPortablePdbTesterCommon.cs b/src/tests/ilasm/PortablePdb/IlasmPortablePdbTesterCommon.cs index f31fb44f5f06b6..0b6cb4735bec67 100644 --- a/src/tests/ilasm/PortablePdb/IlasmPortablePdbTesterCommon.cs +++ b/src/tests/ilasm/PortablePdb/IlasmPortablePdbTesterCommon.cs @@ -21,7 +21,7 @@ public static string GetIlasmFullPath(string coreRootVar, string ilasmFile) return ilasmFullPath; } - public static void Assemble(string ilasmFullPath, string ilSrc, string testDir, out string dll, out string pdb) + public static void Assemble(string ilasmFullPath, string ilSrc, string testDir, out string dll, out string pdb, bool deterministic = false) { var currentDirectory = Environment.CurrentDirectory; var ilSrcFullPath = Path.Combine(currentDirectory, testDir, ilSrc); @@ -33,7 +33,7 @@ public static void Assemble(string ilasmFullPath, string ilSrc, string testDir, var dllFullPath = Path.Combine(currentDirectory, testDir, dllFileName); var pdbFullPath = Path.Combine(currentDirectory, testDir, pdbFileName); - var ilasmArgs = $"{CommonIlasmArguments} -output={dllFullPath} {ilSrcFullPath}"; + var ilasmArgs = $"{CommonIlasmArguments}{(deterministic ? " -det" : string.Empty)} -output={dllFullPath} {ilSrcFullPath}"; var ilasmPsi = new ProcessStartInfo { UseShellExecute = false,