Tar reader checksum checking

[tmds]

I noticed this while working on #117410:

runtime/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs

Lines 372 to 377 in 513ff1a

	int checksum = (int)TarHelpers.ParseOctal<uint>(spanChecksum);
	// Zero checksum means the whole header is empty
	if (checksum == 0)
	{
	return null;
	}

There's no reason to treat zero separate from any other checksum value.
I think this code should be removed.

@ericstj @dotnet/area-system-formats-tar wdyt?

[tmds]

Separate issue/question:

I don't find code that verifies the checksum has the expected value for the header that is read. Is this missing?

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-formats-tar
See info in area-owners.md if you want to be subscribed.

[ericstj]

Seems to me like it's just a fast-path to avoid work when the header is known to be empty. If it's not causing any observable bug then I would leave it.

Are you thinking of a case where the checksum is 0 and the header is not empty? Could we craft such a case to prove we have a bug?

(PS: please assign a milestone when removing untriaged - milestone should be where we want to take action. In this case since a bug here would mean data corruption I've assigned 10.0.0)

[tmds]

Seems to me like it's just a fast-path to avoid work when the header is known to be empty.

It doesn't look like that. On a checksum that has a value zero it returns as if there is no entry to be read.

I think the expected behavior is to check the checksum is correct and throw InvalidDataException if it isn't.
Currently, the checksum is never checked.

[am11]

I think the expected behavior is to check the checksum is correct and throw InvalidDataException if it isn't.

Do other tools (tar(1), 7zip etc.) allow invalid checksum during extraction; warn-and-continue behavior? If so, would be nice to add a bail-out switch for lenient extraction.

[tmds]

tar has no options to ignore invalid checksums.

If the checksum is incorrect, do you want to assume the size is correct to move to the next entry.
I'm not sure this is a use-case the TarReader should try to support.