Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<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

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ