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

This is a discussion on Bug#447153: /usr/bin/scp: Fails to notice write errors - Debian ; Hello On 12/11/2007, Colin Watson wrote: Thanks for looking into this. > Here's a reduced test program that exhibits the same problem as scp when > run with a filename on a CIFS mount of a full filesystem: > > ...

+ Reply to Thread
Results 1 to 2 of 2

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

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

    Hello

    On 12/11/2007, Colin Watson wrote:

    Thanks for looking into this.

    > Here's a reduced test program that exhibits the same problem as scp when
    > run with a filename on a CIFS mount of a full filesystem:
    >
    > #include
    > #include
    > #include
    > #include
    > #include
    > #include
    >
    > int main(int argc, char **argv)
    > {
    > int fd;
    > if (argc <= 1) {
    > fprintf(stderr, "Usage: %s filename\n", argv[0]);
    > return 1;
    > }
    > fd = open(argv[1], O_CREAT | O_WRONLY, 0644);
    > if (fd < 0) {
    > perror("open");
    > return 1;
    > }
    > while (write(fd, "x", 1) < 1) {
    > if (errno == EINTR)
    > continue;
    > perror("write");
    > return 1;
    > }
    > if (ftruncate(fd, 1) < 0) {
    > perror("ftruncate");
    > return 1;
    > }
    > if (close(fd) < 0) {
    > perror("close");
    > return 1;
    > }
    > return 0;
    > }
    >
    > No error, but you end up with a one-byte hole rather than either (a) "x"
    > or (b) an error. If you leave out the ftruncate, then close returns
    > ENOSPC. In either case, you get "CIFS VFS: Write2 ret -28, written = 0"
    > in syslog, but the error code doesn't make it to userspace if there's an
    > ftruncate between write and close. Is this a CIFS bug? I can't find
    > anything in the ftruncate documentation that suggests it is allowed to
    > do this; I think that if write claims to have written all the bytes then
    > userspace ought to be able to assume that ftruncate(fd, st_size) is a
    > no-op.
    >
    >
    > 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.

    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 would suggest closing the file, and if it needs
    truncating, reopen and truncate it (or do some voodoo with duplicated
    fds if possible).
    The file could also be truncated in advance, but that would make
    dealing with files that change during transfer even harder. Although
    one cannot guarantee any sane results in that case anyway.

    Thanks

    Michal



    --
    To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST@lists.debian.org
    with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org

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

    On Mon, Nov 12, 2007 at 08:13:42PM +0100, Michal Suchanek wrote:
    > On 12/11/2007, Colin Watson 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@debian.org]



    --
    To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST@lists.debian.org
    with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org

+ Reply to Thread