[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 16 Sep 2011 11:01:34 +0400
From: Vasiliy Kulikov <segoon@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: Re: cpio write(2) return value checks
Solar, Dmitry,
On Fri, Sep 16, 2011 at 06:16 +0400, Solar Designer wrote:
> On Thu, Sep 15, 2011 at 09:15:55PM +0400, Owl CVS (segoon) wrote:
> > Modified Files:
> > cpio-2.10.90-owl-fixes.diff cpio.spec
> > Added Files:
> > cpio-2.10.90-owl-warnings.diff
> > Log Message:
> > Added checks of write(2) and lseek(2) return codes to -owl-fixes patch.
> [...]
> > +@@ -1176,7 +1177,8 @@ sparse_write (int fildes, char *buf, uns
> > + case not_in_zeros :
> > + if (buf_all_zeros (buf, DISKBLOCKSIZE))
> > + {
> > +- write_rc = write (fildes, cur_write_start, write_count);
> > ++ if (write (fildes, cur_write_start, write_count) != write_count)
> > ++ return -1;
> > + seek_count = DISKBLOCKSIZE;
> > + state = in_zeros;
> > + }
> > +@@ -1197,7 +1199,8 @@ sparse_write (int fildes, char *buf, uns
> > + break;
> > +
> > + case not_in_zeros :
> > +- write_rc = write (fildes, cur_write_start, write_count);
> > ++ if (write (fildes, cur_write_start, write_count) != write_count)
> > ++ return -1;
> > + delayed_seek_count = 0;
> > + break;
> > + }
> [...]
> > +@@ -1206,10 +1209,12 @@ sparse_write (int fildes, char *buf, uns
> [...]
> > +- write_rc = write (fildes, buf, leftover_bytes_count);
> > ++ if (write (fildes, buf, leftover_bytes_count) != leftover_bytes_count)
> > ++ return -1;
> > + }
> > + return nbyte;
> > + }
>
> Wouldn't it be better for the code to handle partial writes instead, and
> only treat 0 and -1 returns from write(2) as errors?
>
> Was the write_rc variable unused? If so, perhaps that was the bug.
Yes, I've removed it in cpio-2.10.90-owl-warnings.diff.
> Maybe we need to add a write_loop() function (like we have in some other
> packages) and use it in these places?
>
> Are these issues possibly already dealt with in a newer version of cpio,
> though?
No. In 2.11 there is no check and write_rc is still unused.
> If so, we'd want not to waste time on our own fix (simple
> checks for "!=" and "return -1" like the above would be fine for now).
>
> Also, doesn't the caller to sparse_write() expect a valid errno on a -1
> return value? (I have no idea, I didn't check.) write(2) only sets
> errno when it returns -1; on a partial write, it does not.
Yes, good point.
cpio-2.11 is a bugfix release:
https://www.gnu.org/s/cpio/
Fix mt build.
In copy-in mode, if directory attributes do not permit writing to
it, setting them is delayed until the end of run. This allows to
correctly extract files in such directories.
In copy-in mode, permissions of a directory are restored if it
appears in the file list after files in it (e.g. in listings produced by
find . -depth). This fixes debian bug #458079.
Fix possible memory overflow in the rmt client code (CVE-2010-0624).
(The latter we have fixed in cpio-2.10.90-up-bound.diff).
Probably we should report these to upstream, wait for 2.12 with the
fixes and upgrade to 2.12?
Thanks,
--
Vasiliy
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
