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 18:24:16 +0200
From: Daniel CegieĊ‚ka <daniel.cegielka@...il.com>
To: owl-dev@...ts.openwall.com
Subject: Re: cpio write(2) return value checks

Can you check cpio-2.10.90-owl-fixes.diff?
I think there is a conflict with cpio-2.9-alt-open-O_EXCL.diff.

regards,
daniel


2011/9/16 Vasiliy Kulikov <segoon@...nwall.com>:
> 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
>

[ CONTENT OF TYPE application/octet-stream SKIPPED ]

Powered by blists - more mailing lists

Your e-mail address:

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