bug#11100: Racy code in copy.c

[Jim Meyering]

NeilBrown wrote:
> On Tue, 27 Mar 2012 15:40:25 +0200 Jim Meyering <address@hidden> wrote:
>
>> Philipp Thomas wrote:
>> > I'd like to pass on observations from my collegue Neil Brown:
>> >
>> > in src/copy.c, copy_reg() is passed "bool *new_dst".
>> >
>> > This is 'false' if the file already exists, in which case it attempts to
>> > open the file with O_WRONLY | O_TRUNC | O_BINARY.
>> > If it is 'true', only then does it use O_CREAT (and others).
>> >
>> > Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
>> > on the file succeeds.  The above mentioned code assumes that the file still
>> > exists.  This is racy - particularly for NFS where deletions from other
>> > clients can take a while to appear.
>>
>> Thanks for the report.
>> However, much of what cp does is mandated by the POSIX spec, including
>> that inevitable-looking (POSIX-mandated) TOCTOU race.  Here's part of that
>> spec, from http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html:
...
>> If you can find a way to make cp work sensibly in your specific case,
>> yet without impacting any other use case, please let us know.
>
> The above doesn't specify how you determine if the dest file exists.
> You could do this with stat plus open(O_WRONLY).
> Only try the open on REG files.

It seems that any change here would be working around the non-POSIX
nature of NFS: note that POSIX seems clear[*] that stat must tell us
whether a file exists (barring "additional or alternate file access
control mechanisms" which aren't an issue here).

I.e., this is an RFE: avoid NFS races due to inherent
POSIX-non-compliance of NFS, rather than a bug.

>        if ((use_stat
> -           ? stat (dst_name, &dst_sb)
> +           ? (stat (dst_name, &dst_sb) < 0 ? -1 :
> +           (fd = open (dst_name, O_WRONLY)) < 0 ? -1 : 0)
>             : lstat (dst_name, &dst_sb))
>            != 0)

At first glance, that might be reasonable: the additional open
is incurred only after a failed stat.
I'll look more closely in a week or two if no one else investigates.

One thing that would help a lot is a test case (even using gdb, strace,
stap, inotify, fuse, etc.) that consistently triggers the failure without
the need for an NFS set-up.

> If the stat fails, or the open fails with ENOENT, then the file doesn't
> exist, otherwise assume it does.
> Keep the file descriptor around (if there is one) and then the "actions
> equivalent to open() ..." would be
>    if (fd < 0)
>          fd = open(pathname, O_WRONLY | O_TRUNC);
>    else
>          ftruncate(fd, 0);
>
>
> I started making a patch - but it got messy quickly.  Too many conditional
> calls to close().
> And at over 1000 lines, copy_internal was too big for me to work with :-(

[*] excerpt from a possibly dated spec:

The stat() function shall obtain information about the named file and
write it to the area pointed to by the buf argument. The path argument
points to a pathname naming a file. Read, write, or execute permission
of the named file is not required. An implementation that provides
additional or alternate file access control mechanisms may, under
implementation-defined conditions, cause stat() to fail. In particular,
the system may deny the existence of the file specified by path.

    If the named file is a symbolic link, the stat() function shall
    continue pathname resolution using the contents of the symbolic link,
    and shall return information pertaining to the resulting file if the file
    exists. The buf argument is a pointer to a stat structure, as defined
    in the <sys/stat.h> header, into which information is placed concerning
    the file. The stat() function shall update any time-related fields
    (as described in XBD Section 4.8, on page 109), before writing into the
    stat structure.

    If the named file is a shared memory object, ...

    If the named file is a typed memory object, ...