owl-dev

server OS

Message-ID: <20110917072318.GA19962@openwall.com>
Date: Sat, 17 Sep 2011 11:23:18 +0400
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: Re: cpio write(2) return value checks

Vasiliy, Dmitry -

On Fri, Sep 16, 2011 at 11:01:34AM +0400, Vasiliy Kulikov wrote:
> No.  In 2.11 there is no check and write_rc is still unused.

I just took a look at the code in context, in 2.11.  sparse_write() is
called from just one place:

  if (sparse_flag)
    bytes_written = sparse_write (out_des, output_buffer, output_size);
  else
    bytes_written = write (out_des, output_buffer, output_size);

  if (bytes_written != output_size)
    {
      error (1, errno, _("write error"));
    }

This treats any non-full write as error, yet it expects errno to be
set in all such cases - which is also wrong for plain write(2), which
may also be called from here.  So this piece quoted above is buggy on
its own, irrespective of what sparse_write() does or does not do.

sparse_write() is defined to have the same return value semantics as
write(2), but with no mention of errno:

/* Write NBYTE bytes from BUF to remote tape connection FILDES.
   Return the number of bytes written on success, -1 on error.  */

Given the calling code, I think it should fully match write(2)'s return
value and errno semantics.  (And the calling code should also be fixed.)

The patch to have sparse_write() return -1 on partial writes is thus
wrong.  Instead, we need to propagate the write_rc values (adding them
up if multiple calls to write(2) are made and they return other than -1)
into the function's return value.

> Probably we should report these to upstream, wait for 2.12 with the
> fixes and upgrade to 2.12?

Maybe, but it's been 1.5 years since 2.11, so it might take a long while
until there's 2.12 as well.  We can ask Sergey for his preference,
though - who is to patch these bugs (we or him), whether he'd accept a
patch from us (I think this stuff is too trivial to be subject to
copyright, yet he might have concerns of this nature), and when he
expects to make a new release.  Even without a release, we can use a
patch backported from upstream, though.  In fact, it's a good idea to
check the current tree - maybe something has already changed in this
area since 2.11...

...and I just did: no, this doesn't appear to have been fixed yet.
There were some post-2.11 commits in July and August 2010 only, and none
look relevant.

http://git.savannah.gnu.org/cgit/cpio.git/log/

Vasiliy, can you bring this up on bug-cpio, perhaps referring to this
owl-dev posting (post its URL)?  Please keep me CC'ed.

https://lists.gnu.org/mailman/listinfo/bug-cpio
http://lists.gnu.org/archive/html/bug-cpio/

Thanks,

Alexander