Skip to content

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Aug 11, 2025

fixes #117455.

TarReader now throws when encountering checksum failures.

Draft beause there are test failures and discussion is needed.

@rzikm

This comment was marked as outdated.

@rzikm
Copy link
Member Author

rzikm commented Aug 11, 2025

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

node-tar$ tar xf bad-cksum.ta
r -C test/
tar: Skipping to next header
tar: Exiting with failure status due to previous errors
node-tar$ ls test/
one-byte.txt
node-tar$ cat test/one-byte.txt
a

❯ tar xf .\bad-cksum.tar -C .\test\
tar.exe: Damaged tar archive
tar.exe: Retrying...
tar.exe: Damaged tar archive
tar.exe: Retrying..

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.

@rzikm rzikm added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Oct 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 7, 2025
Copy link
Contributor

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@rzikm rzikm reopened this Oct 7, 2025
@rzikm rzikm force-pushed the 117455-Tar-reader-checksum-checking branch from b3f26e8 to ee81e70 Compare October 10, 2025 13:36
@rzikm rzikm marked this pull request as ready for review October 10, 2025 13:37
@Copilot Copilot AI review requested due to automatic review settings October 10, 2025 13:37
Copy link
Contributor

@Copilot Copilot AI left a 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

@ericstj ericstj added this to the 11.0.0 milestone Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Formats.Tar breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tar reader checksum checking

3 participants