[Toybox] [PATCH] tar: Fix support for long symbolic links

[Rob Landley]

On 3/19/19 6:38 PM, Daniel Mentz via Toybox wrote:
> Similar to commit 28711d30 ("toybox: tar: Fix support for long names"),
> fix the handling of symbolic links with target names longer than 100
> characters.

The TODO entries are there for a reason. :)

Sorry this is awkward, but commands in "pending" haven't gone through proper
review yet, and unfortunately the tar tests in the test suite don't provide
particularly thorough regression tests. I'm working on improvements to both,

> For target names longer than 100 characters, we store the first 100
> characters in the link field, and the function write_longname() takes
> care of storing the complete target name. The strncpy() function is the
> right tool for this copy operation. Previously, xstrncpy() has been used
> which called error_exit() in cases where the target name was longer than
> 100 characters. However, a truncated or non-null-terminated string is
> not a concern, because the tar program handles this situation correctly
> while reading tar archives.
> 
> Previously, we saw the following error message:
> 
> tar: '<name of long target>' > 100 bytes
> ---
>  toys/pending/tar.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/toys/pending/tar.c b/toys/pending/tar.c
> index 97e699b4..8683fc35 100644
> --- a/toys/pending/tar.c
> +++ b/toys/pending/tar.c
> @@ -209,11 +209,9 @@ static void add_file(char **nam, struct stat *st)
>  // TODO: test preserve symlink ownership
>      hdr.type = '1'+i;
>      if (!(lnk = i ? xreadlink(name) : node->arg)) return perror_msg("readlink");
> -// TODO: does this need NUL terminator?
>      if (strlen(lnk) > sizeof(hdr.link))

The todo item you erased there was a note to self about checking what happens
when a value fits exactly in the field and thus there is no NUL terminator, and
whether the read side was handling that correctly. (Similarly, in theory a
tarball of size 0777777777777 could be use that format without jumping up to the
base 256 format, I wanted to fake up one of those and see what the gnu and
busybox ones did with it. Not that I plan to generate them, but I wanted to know
if I should handle them or error out.)

>        write_longname(lnk, 'K'); //write longname LINK
> -// TODO: this will error_exit() if too long, not truncate.
> -    xstrncpy(hdr.link, lnk, sizeof(hdr.link));
> +    strncpy(hdr.link, lnk, sizeof(hdr.link));

That todo was because it should be strlcpy(), but unfortunately Ulrich Drepper
was an asshole and the committee that replaced him is very, very slow. As in
this was 2014: https://lwn.net/Articles/612244/

That said, similar "what if it fits exactly without space for NUL" todo item,
from 2 lines above and making sure the read side handles it properly, and that
there's entries in the test suite checking that corner case...

Rob