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 <[email protected]> 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 > [email protected] > http://lists.landley.net/listinfo.cgi/toybox-landley.net > _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
