[Toybox] [PATCH] Fix 'cp -R' handling of symlinks

[enh]

it's not just cp(1). there are several other incorrect calls to
readlink/readlinkat.

stat(1), for example:

# stat / /system/bin/stat
  File: `/'
  Size: 0 Blocks: 0 IO Blocks: 4096 directory
Device: 1h Inode: 1 Links: 16
Access: (755/drwxr-xr-x) Uid: (0/    root) Gid: (0/    root)
Access: 1970-01-01 00:00:00.010000000
Modify: 2015-01-08 18:55:35.739999999
Change: 2015-01-08 18:55:35.739999999
  File: `/system/bin/stat' -> `toybox1-08 18:55:35'
  Size: 6 Blocks: 0 IO Blocks: 4096 symbolic link
Device: 1030dh Inode: 341 Links: 1
Access: (755/lrwxr-xr-x) Uid: (0/    root) Gid: (2000/    root)
Access: 2015-01-08 18:44:26.000000000
Modify: 2015-01-08 18:44:26.000000000
Change: 2015-01-08 18:44:26.000000000

lspci(1) looks wrong by inspection, though i don't have an example failure.

readlink(1) and tar(1) are fine because they use xreadlink, and
cpio(1) only writes the number of bytes returned by readlink(2). pwdx
manually terminates the string.

the dirtree and xabspath helpers both manually terminate, though they
seem to have a 4KiB fixed-sized buffer, unlike xreadlink which doesn't
care how large your link target is.

we should probably make more use of xreadlink.

On Thu, Jan 8, 2015 at 7:03 AM, Stephen Mcgruer <smcgruer at google.com> wrote:
> Currently 'cp -R' incorrectly handles symlinks in the source directory, as
> it reuses the same global buffer (toybuf) for successive pairs of
> 'readlinkat', 'symlinkat'. The issue is that readlinkat does not append a
> null byte to the buffer, so longer symlink paths 'stick around' in toybuf
> and corrupt later, shorter paths.
>
> Example reproduction (before this patch):
>
> $ mkdir targets_dir
> $ touch targets_dir/10-a-very-long-file-name.conf
> targets_dir/20-shorter.conf
> $ mkdir from_dir && cd from_dir
> $ ln -s ../targets_dir/10-a-very-long-file-name.conf
> 10-a-very-long-file-name.conf
> $ ln -s ../targets_dir/20-shorter.conf 20-shorter.conf
> $ cd ..
> $ ls -l from_dir/
> lrwxrwxrwx 1 smcgruer eng 44 Jan  8 09:37 10-a-very-long-file-name.conf ->
> ../targets_dir/10-a-very-long-file-name.conf
> lrwxrwxrwx 1 smcgruer eng 30 Jan  8 09:37 20-shorter.conf ->
> ../targets_dir/20-shorter.conf
> $ toybox cp -r from_dir/ to_dir/
> $ ls -l to_dir/
> total 0
> lrwxrwxrwx 1 smcgruer eng 44 Jan  8 09:38 10-a-very-long-file-name.conf ->
> ../targets_dir/10-a-very-long-file-name.conf
> lrwxrwxrwx 1 smcgruer eng 44 Jan  8 09:38 20-shorter.conf ->
> ../targets_dir/20-shorter.conffile-name.conf
>
> To fix this, I just inserted a null-byte using the return value of
> readlinkat (which returns the number of bytes written). Note that this is
> safe because the existing code first checks that sizeof(toybuf) > i. It's a
> little messy, but I was trying to avoid unrolling the ternary into multiple
> layers of conditionals. Perhaps someone on the list can think of a better
> solution!
>
> Thanks,
> Stephen
>
> Patch:
>
> diff --git a/toys/posix/cp.c b/toys/posix/cp.c
> --- a/toys/posix/cp.c
> +++ b/toys/posix/cp.c
> @@ -231,7 +231,8 @@
>          // make symlink, or make block/char/fifo/socket
>          if (S_ISLNK(try->st.st_mode)
>              ? (0 < (i = readlinkat(tfd, try->name, toybuf, sizeof(toybuf)))
> &&
> -               sizeof(toybuf) > i && !symlinkat(toybuf, cfd, catch))
> +               sizeof(toybuf) > i && ((toybuf[i] = 0) == 0) &&
> +               !symlinkat(toybuf, cfd, catch))
>              : !mknodat(cfd, catch, try->st.st_mode, try->st.st_rdev))
>          {
>            err = 0;
>
>
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>

 1420786044.0