Re: [hackers] [sbase][PATCH] tar: bug fix and compatibility improvements

From: Roberto E. Vargas Caballero <k0ga_AT_shike2.net>
Date: Wed, 30 Apr 2025 11:02:39 +0200

Hi,

On Tue, Apr 29, 2025 at 10:20:12PM +1000, Xan Phung wrote:
> 3. Command line argument compatibility improvements: 'c', 'x', 't'
> args are accepted without needing a hyphen in front. The '-p'
> flag is also accepted but is no-op (as it is the normal behaviour
> of sbase tar anyway, and allows sbase tar to be used in scripts
> specifying this flag). Directory tree member extraction is also
> supported by this patch.

I suppose tar was only accepting the hypen version because this is
what the POSIX guidelines mandates, but as tar is not anymore part
of POSIX I think it is ok to accept the no hypen idiom (not sure
if there are still people using it anyway).

> -#define BLKSIZ 512
> +#define BLKSIZ (sizeof (struct header))

For me it is ok, this change, but then I would add an assert to ensure
that a block is actually 512 (just in case someone forgets it in the
future).

> +static long
> +chksum(struct header *h)
> +{
> + long s2, i;
> +
> + memset(h->chksum, ' ', sizeof(h->chksum));
> + for (i = 0, s2 = 0; i < sizeof(*h); i++)
> + s2 += *((unsigned char *)h + i);
> + return s2;
> +}
> +

I think s2 and chksum should be unsigned long (yes, I know that 256
unsigned chars cannot be bigger than 65536, but I think the unsigned
would make more sense here). In fact, we use this function to pass
parameters to putoctal() that receives a unsigned, so maybe it is
just better to use unsigned. Giving the definition of the previous
BLKSIZ macro, maybe we can write something like:

static unsigned
chksum(struct header *h)
{
        unsigned sum;
        unsigned char *p, *s;

        p = s = (unsigned char *) h;
        for (sum = 0; p < &s[BLKSIZ]; sum += *p++)
                ;
        return sum;
}

(or any other variation of the for loop, maybe updating sum in the
body) What do you think?


> _AT_@ -435,9 +432,9 @@ sanitize(struct header *h)
> static void
> chktar(struct header *h)
> {
> - char tmp[8], *err, *p = (char *)h;
> const char *reason;
> - long s1, s2, i;
> + char tmp[8], *err;
> + long s1, i;

Due to the changes in chksum() them s1 should be unsigned, and
maybe we should use the function strtonum() from libutil
instead of using strtol() to parse the checkusm from the
tar file.

> +
> + /* ustar path is copied into fname if no L header (ie: q is NULL) */
> + if (q) q = NULL; else {
> + n = !h->prefix[0] ? 0 :
> + snprintf(fname, l, "%.*s/", (int)sizeof h->prefix, h->prefix);
> + snprintf(fname + n, l - n, "%.*s", (int)sizeof h->name, h->name);
> + }

The style used in suckless in general for braces is the one used
in the linux kernel, omitting the braces only if both parts of
the if/else are composed of only 1 line, and putting the body statement
in its own line. Also, I think that ternary is better expressed using
an if statement. So, I think it should be something like:

        if (q) {
                q = NULL;
        else {
                n = 0;
                if (h->prefix[0])
                        snprintf(fname, l, "%.*s/", (int)sizeof h->prefix, h->prefix);
                snprintf(fname + n, l - n, "%.*s", (int)sizeof h->name, h->name);
        }


> + /* if argc > 0 then only extract the given files */
> + if (argc) {
> + for (i = 0; i < argc; i++) {
> + n = strlen(argv[i]);
> + if (!strncmp(argv[i], fname, n))
> + if (strchr("/", fname[n]) || argv[i][n-1] == '/') break;

Same here, the break should have its own line.

> switch (mode) {
> case 'c':
> + if (!argc) usage();

Same here. Usage() should have its own line.


In general I like the patch, some minor nits. I am going to wait for your
reply to the few small things that I pointed, and see if someone else has
some concerns.

Regards,
Received on Wed Apr 30 2025 - 11:02:39 CEST