tarfs: Fix checksum calculation.
ClosedPublic
Actions

Authored by des on Mar 5 2024, 11:32 AM.

Details

Reviewers

sjg
imp

Group Reviewers

Klara
tests

Commits

rG0118b0c8e58a: tarfs: Fix checksum calculation.

Summary

The checksum code assumed that struct ustar_header filled an entire
block and calculcated the checksum based on the size of the structure.
The header is in fact only 500 bytes long while the checksum covers
the entire block (“logical record” in POSIX terms). Add padding and
an assertion, and clean up the checksum code.

MFC after: 3 days
Sponsored by: Juniper Networks, Inc.
Sponsored by: Klara, Inc.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

des created this revision.Mar 5 2024, 11:32 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 5 2024, 11:32 AM

des requested review of this revision.Mar 5 2024, 11:32 AM

des added a child revision: D44227: tarfs: Factor out common test code..

Harbormaster completed remote builds in B56443: Diff 135405.Mar 5 2024, 11:32 AM

des mentioned this in D44166: tarfs: Improve validation of numeric fields..Mar 5 2024, 11:33 AM

des added a parent revision: D44203: tarfs: Remove unnecessary hack and obsolete comment..Mar 5 2024, 11:34 AM

des added a reviewer: tests.

imp added inline comments.Mar 5 2024, 1:26 PM

sys/fs/tarfs/tarfs_vfsops.c
59	_Static_assert is the new preferred spelling of this. Looks like you are changing all of them, so maybe consider updating.
208	32bit systems?

des marked an inline comment as done.Mar 5 2024, 5:50 PM

des added inline comments.

sys/fs/tarfs/tarfs_vfsops.c
208	It's a straight sum of all the bytes in the block, so the highest possible value is 512 * 255, which is well within the range of `unsigned long`.

des marked 2 inline comments as done.Mar 5 2024, 10:13 PM

des added inline comments.

sys/fs/tarfs/tarfs_vfsops.c
59	I'd rather leave well enough alone until we gain support for the C23 variant.

des marked an inline comment as done.Mar 6 2024, 1:59 PM

imp accepted this revision.Mar 6 2024, 3:37 PM

imp added inline comments.

sys/fs/tarfs/tarfs_vfsops.c
59	fair enough.... thought there's been rumblings of someone doing a CTASSERT -> _Static_assert drive-by... the c23 variant support is likely some way off...

This revision is now accepted and ready to land.Mar 6 2024, 3:37 PM

des marked an inline comment as done.Mar 6 2024, 4:12 PM

des added inline comments.

sys/fs/tarfs/tarfs_vfsops.c
59	Unlike `CTASSERT()` and C23 `static_assert()`, C11 `_Static_assert()` requires a second argument which is used as error message if the assertion fails. A mechanical conversion would be unable to provide a meaningful error message and would thus only serve to make the code both less readable and less useful.