Re: [PATCH] archive: Store checksum correctly

From

René Scharfe <l.s.r@web.de>

Date

Jul 23, 2019, 20:08 UTC

Message-ID

<9a0c89e4-b216-86f1-c721-c54fb2ee0a10@web.de>

In-Reply-To

<14410e00-4701-40d0-6960-e481fea50ed0@web.de>

Am 23.07.19 um 21:38 schrieb René Scharfe:

> is_checksum_valid() in
> https://github.com/AgentD/squashfs-tools-ng/blob/master/lib/tar/checksum.c
> compares the formatted checksum byte-by-byte.  That seems
> unnecessarily strict.  Parsing and comparing the numerical value
> of the checksum would be more forgiving, better adhere to POSIX and
> might be a tiny bit quicker.

(Side note: I'm a bit surprised that GCC 8.3 adds the eight spaces one
 by one in the middle loop with -O2..)

---
 lib/tar/checksum.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/tar/checksum.c b/lib/tar/checksum.c
index a2a101a..af94ab4 100644
--- a/lib/tar/checksum.c
+++ b/lib/tar/checksum.c
@@ -1,15 +1,27 @@
 /* SPDX-License-Identifier: GPL-3.0-or-later */
 #include "internal.h"

-void update_checksum(tar_header_t *hdr)
+static unsigned int get_checksum(const tar_header_t *hdr)
 {
+	const unsigned char *header_start = (const unsigned char *)hdr;
+	const unsigned char *chksum_start = (const unsigned char *)hdr->chksum;
+	const unsigned char *header_end = header_start + sizeof(*hdr);
+	const unsigned char *chksum_end = chksum_start + sizeof(hdr->chksum);
+	const unsigned char *p;
 	unsigned int chksum = 0;
-	size_t i;

-	memset(hdr->chksum, ' ', sizeof(hdr->chksum));
+	for (p = header_start; p < chksum_start; p++)
+		chksum += *p;
+	for (; p < chksum_end; p++)
+		chksum += ' ';
+	for (; p < header_end; p++)
+		chksum += *p;
+	return chksum;
+}

-	for (i = 0; i < sizeof(*hdr); ++i)
-		chksum += ((unsigned char *)hdr)[i];
+void update_checksum(tar_header_t *hdr)
+{
+	unsigned int chksum = get_checksum(hdr);

 	sprintf(hdr->chksum, "%06o", chksum);
 	hdr->chksum[6] = '\0';
@@ -18,9 +30,10 @@ void update_checksum(tar_header_t *hdr)

 bool is_checksum_valid(const tar_header_t *hdr)
 {
-	tar_header_t copy;
+	unsigned int calculated_chksum = get_checksum(hdr);
+	uint64_t read_chksum;

-	memcpy(&copy, hdr, sizeof(*hdr));
-	update_checksum(&copy);
-	return memcmp(hdr, &copy, sizeof(*hdr)) == 0;
+	if (read_octal(hdr->chksum, sizeof(hdr->chksum), &read_chksum))
+		return 0;
+	return read_chksum == calculated_chksum;
 }
--
2.22.0