Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Fri, 16 Sep 2011 06:16:07 +0400
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: cpio write(2) return value checks

Vasiliy, Dmitry -

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.

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?  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.

Thanks,

Alexander

Powered by blists - more mailing lists

Your e-mail address:

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