-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Check checksum when reading TAR archives #118577
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
Check checksum when reading TAR archives #118577
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
There seem to be two test files where we expect to progress even with invalid checksum? node-tar/bad-cksum.tar - this one seems to be extracted correctly with GNU tar but will fail with bsdtar on windows golang_tar/issue12435.tar - this file seems to be garbage, null bytes in checksum. System.Formats.Tar.Tests.TarWriter_Tests.Verify_Compatibility_RegularFile_EmptyFile_NoSizeStored tests seems to be editing some header data, so might just need fixing the checksum in the test data. |
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
b3f26e8 to
ee81e70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds checksum validation to TAR archive reading to improve data integrity. The TarReader now throws InvalidDataException when encountering checksum failures instead of silently ignoring them.
Key changes:
- Adds checksum validation logic to the TAR header reading process
- Updates tests to expect exceptions instead of null returns for invalid checksums
- Adds new error message resource for checksum validation failures
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs | Implements checksum validation and adds helper method to calculate header checksums |
| src/libraries/System.Formats.Tar/src/Resources/Strings.resx | Adds new error message resource for checksum validation failures |
| src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs | Adds comprehensive test for invalid checksum scenarios |
| src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Tests.cs | Updates existing tests to expect exceptions for checksum failures |
| src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Async.Tests.cs | Updates async tests to expect exceptions for checksum failures |
| src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.Tests.cs | Fixes checksum in existing test to maintain validity |
| src/libraries/System.Formats.Tar/tests/TarTestsBase.cs | Removes "bad-cksum" test case from exclusion list |
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Outdated
Show resolved
Hide resolved
📋 Breaking Change Documentation RequiredCreate a breaking change issue with AI-generated content Generated by Breaking Change Documentation Tool - 2025-10-15 09:30:09 |
…ts.cs Co-authored-by: Copilot <[email protected]>
|
@ericstj I vectorized the implementation and ran some benchmarks (see PR description), there may be some small impact on read, but we are getting significant gains on the write path. PTAL, and if it is fine then let's merge this. |
63eb2fb to
2534092
Compare
ericstj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you for the perf tests.
|
Created breaking change doc: dotnet/docs#49394 |
fixes #117455.
TarReader now throws when encountering checksum failures.
This PR also vectorizes checksum calculation already present on the Write path of the TarHeader, which seems to speed-up some benchmarks
Benchmark data