[PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

Two issues found in current CVS:

1. Booting a grub2-mkrescue floppy crashes if 
"(memdisk)/boot/grub/grub.cfg" does not exist. This is because 
grub_cpio_open does not set grub_errno if a file does not exist.

2. The cpio format may not work. Header scan finishes early if data size 
is empty (directory, empty file). The cpio format uses the name 
"TRAILER!!!" to mark the last block.

This patch fixes both issues.


"grub-mkrescue --image-type=floppy" now works with both tar and cpio 
memdisk. Cpio was tested with this change to grub-mkrescue:

-  tar -C ${aux_dir} -cf ${memdisk_img} boot
+  ( cd ${aux_dir} && find boot | cpio -o > ${memdisk_img} )


Open issues not fixed in this patch:

- Directory detection relies on a trailing '/' in path name. This works 
for typical tar files, but not for cpio. As a consequence, tab 
completion and "ls -l" are not correct. The "mode" in the header should 
be checked instead.

- CPIO would not work on big endian architectures yet.


Christian

2008-02-05  Christian Franke  <franke@computer.org>

	* fs/cpio.c (grub_cpio_find_file): Return GRUB_ERR_NONE
	and (*ofs = 0) instead of GRUB_ERR_FILE_NOT_FOUND on last
	block of a cpio or tar stream.
	Check for "TRAILER!!!" instead of any empty data
	block to detect last block of a cpio stream.
	(grub_cpio_dir): Fix constness of variable np.
	(grub_cpio_open): Return GRUB_ERR_FILE_NOT_FOUND if
	cpio or tar trailer is detected.  This fixes a crash
	on open of a non existing file.



[-- Attachment #2: grub2-cpio-eof.patch --]
[-- Type: text/x-patch, Size: 1530 bytes --]

--- grub2.orig/fs/cpio.c	2008-02-03 20:29:51.718750000 +0100
+++ grub2/fs/cpio.c	2008-02-05 22:59:50.031250000 +0100
@@ -98,11 +98,6 @@ grub_cpio_find_file (struct grub_cpio_da
 	return grub_error (GRUB_ERR_BAD_FS, "Invalid cpio archive");
 
       data->size = (((grub_uint32_t) hd.filesize_1) << 16) + hd.filesize_2;
-      if (data->size == 0)
-	{
-	  *ofs = 0;
-	  return GRUB_ERR_FILE_NOT_FOUND;
-	}
 
       if (hd.namesize & 1)
 	hd.namesize++;
@@ -117,6 +112,13 @@ grub_cpio_find_file (struct grub_cpio_da
 	  return grub_errno;
 	}
 
+      if (data->size == 0 && hd.mode == 0 && hd.namesize == 11 + 1
+	  && ! grub_memcmp(*name, "TRAILER!!!", 11))
+	{
+	  *ofs = 0;
+	  return GRUB_ERR_NONE;
+	}
+
       data->dofs = data->hofs + sizeof (hd) + hd.namesize;
       *ofs = data->dofs + data->size;
       if (data->size & 1)
@@ -133,7 +135,7 @@ grub_cpio_find_file (struct grub_cpio_da
       if (!hd.name[0])
 	{
 	  *ofs = 0;
-	  return GRUB_ERR_FILE_NOT_FOUND;
+	  return GRUB_ERR_NONE;
 	}
 
       if (grub_memcmp (hd.magic, MAGIC_USTAR, sizeof (MAGIC_USTAR) - 1))
@@ -188,7 +190,8 @@ grub_cpio_dir (grub_device_t device, con
 {
   struct grub_cpio_data *data;
   grub_uint32_t ofs;
-  char *prev, *name, *np;
+  char *prev, *name;
+  const char *np;
   int len;
 
 #ifndef GRUB_UTIL
@@ -275,7 +278,10 @@ grub_cpio_open (grub_file_t file, const 
 	goto fail;
 
       if (!ofs)
-	break;
+	{
+	  grub_error (GRUB_ERR_FILE_NOT_FOUND, "file not found");
+	  break;
+	}
 
       if (grub_strcmp (name + 1, fn) == 0)
 	{

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Robert Millan]

Hi Christian,

On Wed, Feb 06, 2008 at 12:02:37AM +0100, Christian Franke wrote:
> Two issues found in current CVS:
> 
> 1. Booting a grub2-mkrescue floppy crashes if 
> "(memdisk)/boot/grub/grub.cfg" does not exist. This is because 
> grub_cpio_open does not set grub_errno if a file does not exist.

I can't reproduce this with qemu:

  ./grub-mkrescue --pkglibdir=`pwd` --grub-mkimage=`pwd`/grub-mkimage --image-type=floppy /tmp/grub-rescue-floppy.img
  qemu -fda /tmp/grub-rescue-floppy.img -boot a

is this supposed to be a spurious problem?

> 2008-02-05  Christian Franke  <franke@computer.org>
> 
> 	* fs/cpio.c (grub_cpio_find_file): Return GRUB_ERR_NONE
> 	and (*ofs = 0) instead of GRUB_ERR_FILE_NOT_FOUND on last

I would suggest "(and set *ofs = 0)" here to make it clearer.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Christian Franke]

Hi Robert,

you wrote:
> On Wed, Feb 06, 2008 at 12:02:37AM +0100, Christian Franke wrote:
> > Two issues found in current CVS:
> > 
> > 1. Booting a grub2-mkrescue floppy crashes if
> > "(memdisk)/boot/grub/grub.cfg" does not exist. This is because
> > grub_cpio_open does not set grub_errno if a file does not exist.
> 
> I can't reproduce this with qemu:
> 
> ./grub-mkrescue --pkglibdir=`pwd` --grub-mkimage=`pwd`/grub-mkimage
> --image-type=floppy /tmp/grub-rescue-floppy.img qemu -fda
> /tmp/grub-rescue-floppy.img -boot a
> 
> is this supposed to be a spurious problem?
> 
> 

This depends on the (random?) contents of the "file" variable.  If a
file does not exist, grub_cpio_open() returns GRUB_ERR_NONE but leaves
the file variable as is.

Accessing some non existent file ("cat (memdisk)/nosuchfile") should
eventually result in a crash.

Christian

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Christian Franke]

Robert Millan wrote:
>   
>> 2008-02-05  Christian Franke  <franke@computer.org>
>>
>> 	* fs/cpio.c (grub_cpio_find_file): Return GRUB_ERR_NONE
>> 	and (*ofs = 0) instead of GRUB_ERR_FILE_NOT_FOUND on last
>>     
>
> I would suggest "(and set *ofs = 0)" here to make it clearer.
>
>   

OK.

2008-02-06  Christian Franke  <franke@computer.org>

	* fs/cpio.c (grub_cpio_find_file): Return GRUB_ERR_NONE
	(and set *ofs = 0) instead of GRUB_ERR_FILE_NOT_FOUND on
	last block of a cpio or tar stream.
	Check for "TRAILER!!!" instead of any empty data
	block to detect last block of a cpio stream.
	(grub_cpio_dir): Fix constness of variable np.
	(grub_cpio_open): Return GRUB_ERR_FILE_NOT_FOUND if
	cpio or tar trailer is detected.  This fixes a crash
	on open of a non existing file.

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Bean]

On Feb 6, 2008 7:02 AM, Christian Franke <Christian.Franke@t-online.de> wrote:
> Two issues found in current CVS:
>
> 1. Booting a grub2-mkrescue floppy crashes if
> "(memdisk)/boot/grub/grub.cfg" does not exist. This is because
> grub_cpio_open does not set grub_errno if a file does not exist.
>
> 2. The cpio format may not work. Header scan finishes early if data size
> is empty (directory, empty file). The cpio format uses the name
> "TRAILER!!!" to mark the last block.
>
> This patch fixes both issues.

Actually, the first version of grub_cpio_find_file returned
GRUB_ERR_FILE_NONE when the end of file is encounter, but then,
somebody find out that opening non existent file have problem, and the
result is changed  to GRUB_ERR_FILE_NOT_FOUND. I guess it still have
problem, the fix you are providing seems ok.

> Open issues not fixed in this patch:
>
> - Directory detection relies on a trailing '/' in path name. This works
> for typical tar files, but not for cpio. As a consequence, tab
> completion and "ls -l" are not correct. The "mode" in the header should
> be checked instead.
>
> - CPIO would not work on big endian architectures yet.
>

yes, i know this problem, i can fix it sometime.

-- 
Bean

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Robert Millan]

On Wed, Feb 06, 2008 at 03:29:45PM +0800, Bean wrote:
> > This patch fixes both issues.
> 
> Actually, the first version of grub_cpio_find_file returned
> GRUB_ERR_FILE_NONE when the end of file is encounter, but then,
> somebody find out that opening non existent file have problem, and the
> result is changed  to GRUB_ERR_FILE_NOT_FOUND. I guess it still have
> problem, the fix you are providing seems ok.

Christian doesn't have write perms.  Bean, since you're more familiar with
this code, will you check that in?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Fix crash on open of nonexisting tar/cpio file, fix cpio trailer detection

[Bean]

On Feb 7, 2008 3:18 AM, Robert Millan <rmh@aybabtu.com> wrote:
> On Wed, Feb 06, 2008 at 03:29:45PM +0800, Bean wrote:
> > > This patch fixes both issues.
> >
> > Actually, the first version of grub_cpio_find_file returned
> > GRUB_ERR_FILE_NONE when the end of file is encounter, but then,
> > somebody find out that opening non existent file have problem, and the
> > result is changed  to GRUB_ERR_FILE_NOT_FOUND. I guess it still have
> > problem, the fix you are providing seems ok.
>
> Christian doesn't have write perms.  Bean, since you're more familiar with
> this code, will you check that in?

ok, committed.

-- 
Bean