bug#11100: Racy code in copy.c

[NeilBrown]

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:
> 
>     For each source_file, the following steps shall be taken:
> 
>         ...
>         If source_file is of type directory, the following steps shall be 
> taken:
> 
>             ...
> 
>         If source_file is of type regular file, the following steps
>         shall be taken:
> 
>             The behavior is unspecified if dest_file exists and was
>             written by a previous step. Otherwise, if dest_file exists,
>             the following steps shall be taken:
> 
>                 If the -i option is in effect, the cp utility shall
>                 write a prompt to the standard error and read a line from
>                 the standard input. If the response is not affirmative,
>                 cp shall do nothing more with source_file and go on to
>                 any remaining files.
> 
>                 A file descriptor for dest_file shall be obtained by
>                 performing actions equivalent to the open() function
>                 defined in the System Interfaces volume of POSIX.1-2008
>                 called using dest_file as the path argument, and the
>                 bitwise-inclusive OR of O_WRONLY and O_TRUNC as the
>                 oflag argument.
> 
>                 If the attempt to obtain a file descriptor fails and the
>                 -f option is in effect, cp shall attempt to remove the
>                 file by performing actions equivalent to the unlink()
>                 function defined in the System Interfaces volume
>                 of POSIX.1-2008 called using dest_file as the path
>                 argument. If this attempt succeeds, cp shall continue
>                 with step 3b.
> 
>             If dest_file does not exist, a file descriptor shall be
>             obtained by performing actions equivalent to the open()
>             function defined in the System Interfaces volume of
>             POSIX.1-2008 called using dest_file as the path argument,
>             and the bitwise-inclusive OR of O_WRONLY and O_CREAT as
>             the oflag argument. The file permission bits of source_file
>             shall be the mode argument.
> 
>             If the attempt to obtain a file descriptor fails, cp shall
>             write a diagnostic message to standard error, do nothing
>             more with source_file, and go on to any remaining files.
> 
>             The contents of source_file shall be written to the file
>             descriptor. Any write errors shall cause cp to write a
>             diagnostic message to standard error and continue to step 3e.
> 
>             The file descriptor shall be closed.
> 
>             The cp utility shall do nothing more with source_file. If
>             a write error occurred in step 3d, it is unspecified if
>             cp continues with any remaining files. If no write error
>             occurred in step 3d, cp shall go on to any remaining files.
> 
> 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.

       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)


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 :-(

NeilBrown

signature.asc
Description: PGP signature