Re: [Bug-tar] [PATCH] --one-top-level: avoid a heap-buffer-overflow

[Jim Meyering]

On Sun, Apr 8, 2018 at 1:53 AM, Sergey Poznyakoff <address@hidden> wrote:
> Jim Meyering <address@hidden> ha escrit:
>
>>   $ : > k && tar cf k.tar k; valgrind tar --one-top-level -tf k.tar
>>
>>   Invalid read of size 1
>>      at 0x421FCF: strip_compression_suffix (suffix.c:107)
>
> Thanks for finding that out. However, the proposed fix doesn't seem
> quite correct to me:
>
>> +      size_t len = (strncmp (name + base_len, ".tar", 4) == 0
>> +                 ? base_len : strlen (name));
>
> This will result in returning entire string, except if it ends in
> ".tar" followed by compression suffix. The idea was, however, to
> strip off the compression suffix *and* eventual ".tar" before it.
>
> I would suggest the following simple fix:
>
> diff --git a/src/suffix.c b/src/suffix.c
> index 66b5694..6cb521a 100644
> --- a/src/suffix.c
> +++ b/src/suffix.c
> @@ -104,7 +104,7 @@ strip_compression_suffix (const char *name)
>
>    if (find_compression_suffix (name, &len))
>      {
> -      if (strncmp (name + len - 4, ".tar", 4) == 0)
> +      if (len > 4 && memcmp (name + len - 4, ".tar", 4) == 0)
>         len -= 4;
>        if (len == 0)
>         return NULL;

I see what you mean. So how would you like a NAME of "a.tar.tar" to be
treated? Seems wrong to remove the second .tar suffix. How about this
change, to make it remove ".tar" only if the just-stripped suffix is
not also ".tar"?

double-tar-suffix.diff
Description: Binary data