Fix deterministic MVID and add PdbChecksum #810
Loading…
Reference in a new issue
No description provided.
Delete branch "ImproveDeterministicMVID"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is a port of https://github.com/mono/cecil/pull/31.
Make deterministic MVID unique even when the only change is in PE headers. Implement deterministic PDB ID generation independent of MVID and implement writing of PdbCheckum debug header entry.
Observable differences (other than the above):
GetDebugHeader(well except for MPDB writer which I didn't change as it's really tricky). Callers should not notice much of a difference if they assumed onlyDisposewould actually write.Module.Write(streamnow requires the stream to be read/write. It already did so for cases where strong name was written, now it requires it also when deterministic MVID is turned on.Deterministic MVID
So far the deterministic MVID was computed as a hash of the IL metadata. It didn't include PE headers.
This means that if there's an assembly which only differs in the PE architecture it would have the same MVID.
This can happen relatively easily for facades (assemblies with only type forwarders) as those are typically
identical across platforms, possibly except for the architecture.
The correct way to calculate MVID is to hash the content of the entire file with the MVID itself and
the strong name signature (if presenet) zeroed out.
This change modified the MVID computation to work that way: Write the entire image with MVID all zeroes,
compute the hash, write the MVID and then compute the strong name.
PdbChecksum
In order for the produced PDB to be fully verifiable the assembly must contain a PdbChecksum
debug header entry which has a cryptographic hash of the associated PDB. The wayt to calculate the checksum
is prescribed as other programs must be able to validate it. See https://github.com/dotnet/runtime/blob/main/docs/design/specs/PE-COFF.md#pdb-checksum-debug-directory-entry-type-19.
This change implements that behavior. Symbols are now written explicitly once all metadata is processed
but before the final assembly image is written. For portable PDBs the full file is written with PDB ID
set to all zeroes. Then the crypto hash is calculated (using SHA256).
The hash is then written into the PdbChecksum debug header entry of the assembly image.
Deterministic Portable PDB ID
So far the PDB ID was set to the same value as MVID. With the fix for MVID described above, this doesn't
work anymore. In case of an embedded PDB, it's not possible to calculate MVID using the hash
with the PDB embedded, as the PDB would now contain the MVID (cycle).
The recommended way to calculate PDB ID is to use the same mechanism as for calculating PDB checksum
and use the first 20 bytes of the has as the PDB ID.
This change implements this by using the first 20 bytes of the hash and sets the PDB ID as the last
change of the portable PDB.
The calculated PDB ID is also then set into the CodeView debug header entry of the assembly.
Added tests for reading and writing both portable and embedded portable PDBs with checksums.
Added test to validate stability of PDB ID across multiple writes.
Added test to validate stability of MVID across multiple writes and the fact that it changes
when just the PE architecture is changed.
This is a fix for https://github.com/dotnet/linker/issues/2203.
Nice! This should fix #610.
Thanks a lot for the PR, I'll review ASAP.
does the mono side have tests that can also be ported over?
@SimonCropp this PR adds quite a few tests: https://github.com/jbevain/cecil/pull/810/files#diff-a834b6b7fab0440c40cd010733a632eec5086a9b39422a5a36f11f1f7dc5ed78
What other tests do you have in mind?
(The way we hit this is actually long way away from any tests - users reported that in some cases symbols are not loading in VS and if we were to add tests for that it would be something like "the assemblies produced by official builds of dotnet/runtime all have valid symbols on the symbol server. We didn't hit this through automated testing - that is a test hole though).
@vitek-karas so sorry. i only glanced at the latest commit in my GH notification emial.
@vitek-karas Thank you for all this work. I spent a bunch of time reading through the changes and I think I'm mostly OK with it.
One request though, I've started some initial work a while ago:
https://github.com/jbevain/cecil/pull/617/files
This introduces a Write method on the debug writer, which makes it a lot clearer than expecting GetDebugHeader to do the writing.
I'd love it if you could incorporate the logic from the initial work into your PR as I think it makes things a bit clearer.
Thanks!
@jbevain Thanks for the review!
I did see your original change and the
Writemethod, but I didn't know what's the compatibility promise on Cecil really. Changing this to use theWriteis technically a breaking change as theISymbolWriteris public and could be provided externally. So if somebody has an implementation ofISymbolWriterit won't compile anymore (just like the tests). Similarly if somebody is using the Cecil implementedISymbolWriterthen it will break - it will compile, but the code will likely not work as intended.If making such a breaking change is fine, I'll be happy to change the implementation to use the
Writemethod as it is definitely much clearer.I think that's an acceptable breakage, I'm not aware of any other implementor :)
Made the changes to add
Writeand change the ordering.Could you please rerun the Linux CI? The failure seems completely unrelated to my change.
Thanks!