Bug#447153: /usr/bin/scp: Fails to notice write errors

[Colin Watson]

On Mon, Nov 12, 2007 at 08:13:42PM +0100, Michal Suchanek wrote:
> On 12/11/2007, Colin Watson <cjwatson at debian.org> wrote:
> > To openssh-unix-dev: does anyone think this is worth a workaround? The
> > ftruncate seems rather unnecessary if we've already written out the
> > required number of bytes anyway. I've attached a patch which only does
> > it if that isn't the case (although I have some trouble seeing how we
> > could ever get to the ftruncate without either writing the required
> > number of bytes or encountering a write error). If people think it's a
> > good idea I'll file it in Bugzilla.
> 
> I do not know the scp code so I might be missing something. However,
> truncating the file might be necessary when there is already a file,
> and it was originally longer.

Whoops! You're quite right; I blame the jetlag. (Though, since current
portable CVS checks whether the file exists before trying to ftruncate
it, simply changing '!exists || S_ISREG(stb.st_mode)' to 'exists &&
S_ISREG(stb.st_mode) && (new file shorter than old file)' would be
another possibility; I can't see why you would want to truncate if it
didn't previously exist, and the only way you can run into this bug if
you're shortening an existing file is if you're copying over the top of
an existing sparse file, which is even more of a crazy corner case than
this already is.

> It looks like a bug either in the kernel or in ftruncate
> documentation. It is certainly true that the write error should get
> reported at some point if it occurs, and a filesystem that chooses to
> not return it on write() should save the errors for close().
> 
> However, using ftruncate() on the file does, in some sense,
> successfully extend the file to the desired length. It looks like such
> mixing of calls was not expected by the fs driver, and may not be well
> defined in general.

I understand your point, and I spent a little while pondering it before
sending my mail. I think that if the write syscall doesn't actually
write the bytes out to the filesystem then that's its own problem. If
the write returns a non-zero number of bytes, ftruncate should behave in
all cases *as if* those bytes have been written to the filesystem, even
if they haven't actually quite been written yet. POSIX is pretty
consistent in this; implementation details of buffering shouldn't be
exposed except where they're explicitly defined to be exposed.

(However, we should be having this debate on the linux-cifs-client list,
not openssh-unix-dev ...)

> I would suggest closing the file, and if it needs
> truncating, reopen and truncate it (or do some voodoo with duplicated
> fds if possible).

Something like that would be another reasonable workaround, yes.
Truncating only if the file already exists and the new one is shorter
seems simpler to me, but I'm not all that bothered about the exact
approach.

Cheers,

-- 
Colin Watson                                       [cjwatson at debian.org]