Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use SHA256 by default in command line compilers #24820

Merged
merged 2 commits into from
Jan 23, 2019
Merged

Conversation

tmat
Copy link
Member

@tmat tmat commented Feb 13, 2018

Customer scenario

The C# and VB command line compilers and build tasks use SHA1 by default for calculating hashes of source files stored in PDBs. It has already been possible to explicitly request SHA256 algorithm by specifying /checksumalgorithm:SHA256 on command line or setting ChecksumAlgorithm property in project file. This change makes SHA256 the default since SHA1 is no longer an acceptable crypto hash algorithm.

Bugs this fixes

Workarounds, if any

Set the algorithm explicitly.

Risk

Small.

Performance impact

Small.

I measured build of Microsoft.CodeAnalysis.dll in several configurations. The difference in compilation time between SHA1 and SHA256 is not measurable in end-to-end scenario. The size difference is 1% for Portable PDBs and none in Windows PDBs (due to section alignment).

Time (s)

  Portable PDB Windows PDB
SHA1 3.6032 3.7272
SHA256 3.6030 3.7356
  99.99% 100.23%

PDB size (B)

  Portable PDB Windows PDB
SHA1 953,792 6,641,152
SHA256 960,884 6,641,152
101% 100%

Method:

1..100 | % {(Measure-Command { &csc /noconfig @CodeAnalysis.rsp args }).TotalSeconds} | Measure-Object -Average

where args are:

/debug:portable /checksumalgorithm:sha1
/debug:portable /checksumalgorithm:sha256
/debug:full /checksumalgorithm:sha1
/debug:full /checksumalgorithm:sha256

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

@tmat tmat requested a review from a team as a code owner February 13, 2018 22:38
@tmat
Copy link
Member Author

tmat commented Feb 14, 2018

@jinujoseph @MattGertz Looks like the extra processing time of SHA256 is within the noise. The Portable PDB size increase of 1% is acceptable imo. The Windows PDB did not grow since it contains a lot of padding and alignment that was consumed by the larger hashes.

@MattGertz
Copy link
Contributor

Groovy. @jac009 any concerns? This is in reference to the meeting we had on this transition last week.

@jac009
Copy link

jac009 commented Feb 14, 2018

@MattGertz Very torn on this. Would really like to understand what level of testing for teams that rely on PDBs (debuggers, profilers, Watson etc.) has been done.

@tmat
Copy link
Member Author

tmat commented Feb 14, 2018

@jac009 I'll get that info from the respective teams. The VS debugger has supported SHA256 since one of the Dev14 updates, VS Code and VS Mac Core CLR debugger since day one.

@tmat tmat changed the title Use SHA256 by default in command line compilers WIP: Use SHA256 by default in command line compilers Feb 14, 2018
@tmat
Copy link
Member Author

tmat commented Feb 14, 2018

Before switching to SHA256 by default in the compilers we will first use the explicit switch in all our repositories to flush out any potential issues it might cause.

RepoToolset repos:

Other repos:

@tmat tmat requested review from a team as code owners January 18, 2019 19:36
@tmat tmat changed the title WIP: Use SHA256 by default in command line compilers Use SHA256 by default in command line compilers Jan 18, 2019
@tmat tmat changed the base branch from dev15.7.x to master January 18, 2019 19:36
@tmat tmat modified the milestones: Backlog, 16.0.P3 Jan 22, 2019
@tmat tmat removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 22, 2019
@tmat tmat merged commit ad9a917 into dotnet:master Jan 23, 2019
@tmat tmat deleted the SHA256 branch January 23, 2019 18:04
xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019
* Use SHA256 by default in command line compilers (/checksumalgorithm switch)

* Update XLF
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area-Compilers Tenet-Compliance Violation of compliance with things like signing, security, legality, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants