Re: [PATCH] archive: Store checksum correctly

[René Scharfe]

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.

I mean something like the patch below.  Code and text size are bigger,
but it's more lenient and writes less.  Untested.

(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