Re: cpio - covscan issues

[Kamil Dudka]

On Thursday, April 8, 2021 4:07:14 PM CEST Martin Simmons wrote:
> >>>>> On Thu, 08 Apr 2021 13:37:13 +0200, Kamil Dudka said:
> > On Thursday, April 8, 2021 12:33:11 PM CEST Ondrej Dubaj wrote:
> > > On Thu, Apr 8, 2021 at 12:02 PM Kamil Dudka <kdudka@redhat.com> wrote:
> > > > On Thursday, April 8, 2021 9:47:05 AM CEST Ondrej Dubaj wrote:
> > > > > Hello,
> > > > > 
> > > > > proposing patch for some of the issues found by coverity scan in
> > > > 
> > > > cpio-2.13
> > > > 
> > > > > Patch:
> > > > > 
> > > > > diff --git a/src/tar.c b/src/tar.c
> > > > > index 99ef8a2..a5873e7 100644
> > > > > --- a/src/tar.c
> > > > > +++ b/src/tar.c
> > > > > @@ -146,6 +146,7 @@ write_out_tar_header (struct cpio_file_stat
> > > > 
> > > > *file_hdr,
> > > > 
> > > > > int out_des)
> > > > > 
> > > > >    name_len = strlen (file_hdr->c_name);
> > > > >    if (name_len <= TARNAMESIZE)
> > > > >    
> > > > >      {
> > > > > 
> > > > > +      memset(tar_hdr->name, '\0', name_len+1);
> > > > > 
> > > > >        strncpy (tar_hdr->name, file_hdr->c_name, name_len);
> > > > >      
> > > > >      }
> > > > >    
> > > > >    else
> > > > 
> > > > This is obviously incorrect because it would write past the
> > > > tar_hdr->name
> > > > array in case (name_len == TARNAMESIZE).
> > > 
> > > Actually you are right, the best option might be:
> > > 
> > > memset(tar_hdr->name, '\0', TARNAMESIZE);
> > 
> > This would not ensure NUL-termination either because the subsequent call
> > to
> > strncpy() might overwrite all the zeros with non-zeros in case (name_len
> > ==
> > 
> > TARNAMESIZE).  I believe this would work better:
> >     strncpy (tar_hdr->name, file_hdr->c_name, name_len);
> >     tar_hdr->name[TARNAMESIZE - 1] = '\0';
> 
> Does it have to be NUL-terminated in the tar header?  GNU tar doesn't
> NUL-terminate filenames with length 100 in the v7 format:
> 
> $ touch
> 012345678901234567890123456789012345678901234567890123456789012345678901234
> 567890123456789012345678x $ tar -c --format=v7 -f -
> 012345678901234567890123456789012345678901234567890123456789012345678901234
> 567890123456789012345678x | od -c | head -7 0000000   0   1   2   3   4   5 
>  6   7   8   9   0   1   2   3   4   5 0000020   6   7   8   9   0   1   2 
>  3   4   5   6   7   8   9   0   1 0000040   2   3   4   5   6   7   8   9 
>  0   1   2   3   4   5   6   7 0000060   8   9   0   1   2   3   4   5   6 
>  7   8   9   0   1   2   3 0000100   4   5   6   7   8   9   0   1   2   3 
>  4   5   6   7   8   9 0000120   0   1   2   3   4   5   6   7   8   9   0 
>  1   2   3   4   5 0000140   6   7   8   x   0   0   0   0   6   6   4  \0 
>  0   0   0   1
> 
> The x is the last character of the name in the header, followed by the
> mode 0000664 in octal.
> 
> The code should probably be using memcpy instead of strncpy because the
> header has already been filled with NULs.
> 
> __Martin

Good point.  If NUL-termination is not needed for names consisting of exactly 
TARNAMESIZE bytes, the code is correct as it is.  Using memcpy() instead of 
strncpy(), as you suggest, would help to eliminate the warning.

Kamil