[PATCH][RFC] LAR segfault when adding to a full archive

[Carl-Daniel Hailfinger]

Hi,

Alvar Kusma found a bug in util/lar: If you try to add a file to a full
LAR archive, the LAR utility will segfault. This is reproduced easily by
zerofilling the LAR, then adding anything to it.

Looking at the code, the reason is obvious:
lar_empty_offset() can return an error code (-1). None of the callers
check for an error code, they simply assume the return value is valid.

Preliminary patch follows (and raises a few questions).
Signed-off-by: Carl-Daniel Hailfinger

Index: util/lar/lar.h
===================================================================
--- util/lar/lar.h	(Revision 646)
+++ util/lar/lar.h	(Arbeitskopie)
@@ -58,7 +58,9 @@
 #define BOOTBLOCK_NAME_LEN 16

typedef uint64_t u64;
+typedef int64_t s64;
 typedef uint32_t u32;
+typedef int32_t s32;
 typedef uint8_t  u8;

/* NOTE -- This and the coreboot lar.h may NOT be in sync. Be careful. */
Index: util/lar/stream.c
===================================================================
--- util/lar/stream.c	(Revision 646)
+++ util/lar/stream.c	(Arbeitskopie)
@@ -492,7 +492,7 @@
  * @param lar the LAR archive structure
  * @return The offset of the first chunk of empty space
  */
-static int lar_empty_offset(struct lar *lar)
+static s64 lar_empty_offset(struct lar *lar)
 {
    u32 offset = 0;
    struct lar_header *header;
@@ -508,10 +508,12 @@
    	offset += get_next_offset(header);
    }

-	if (offset >= get_bootblock_offset(lar->size))
+	if (offset >= get_bootblock_offset(lar->size)) {
+		err("No empty space found!\n");	
    	return -1;
+	}

-	return offset;
+	return (s64)offset;
 }

/**
@@ -825,11 +827,16 @@
 int maxsize(struct lar *lar, char *name)
 {
    int size;
-	u32 offset;
+	s64 offset;
    int bootblock_size;

/* Find the beginning of the available space in the LAR */
+#warning We should check all chunks of free space in the LAR. Right now we do not return the maximum size, but the size of the first chunk.
    offset = lar_empty_offset(lar);
+	if (offset < 0) {
+		err("maxsize is negative\n");
+		return offset;
+	}

/* Figure out how big our header will be */
    size = get_bootblock_offset(lar->size) - offset - header_len(name,NULL) - 1;
@@ -878,7 +885,7 @@
    int ret, hlen;
    int pathlen;
    u32 *walk,  csum;
-	u32 offset;
+	s64 offset;

/* Find the beginning of the available space in the LAR */
    offset = lar_empty_offset(lar);
@@ -886,7 +893,8 @@
    /* Figure out how big our header will be */
    hlen = header_len(pathname, &pathlen);

-	if (offset + hlen + complen >= get_bootblock_offset(lar->size)) {
+	if ((offset < 0) || 
+		(offset + hlen + complen >= get_bootblock_offset(lar->size))) {
    	err("Not enough room in the LAR to add the file.\n");
    	return -1;
    }

[PATCH][RFC] LAR segfault when adding to a full archive

[Peter Stuge]

On Fri, Mar 28, 2008 at 12:10:16AM +0100, Carl-Daniel Hailfinger wrote:
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Peter Stuge
> +#warning We should check all chunks of free space in the LAR. Right now we do not return the maximum size, but the size of the first chunk.

Yeah, LAR isn't completely adult yet, but this is good for now.

//Peter

[PATCH][RFC] LAR segfault when adding to a full archive

[Carl-Daniel Hailfinger]

On 28.03.2008 00:34, Peter Stuge wrote:
> On Fri, Mar 28, 2008 at 12:10:16AM +0100, Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Peter Stuge
> > +#warning We should check all chunks of free space in the LAR. Right now we do not return the maximum size, but the size of the first chunk.
>
> Yeah, LAR isn't completely adult yet, but this is good for now.

Thanks, r647.

Regards,
Carl-Daniel

[PATCH][RFC] LAR segfault when adding to a full archive

[Stefan Reinauer]

NACK as long as the code adds #warnings. Please fix the issues and resubmit.

Carl-Daniel Hailfinger wrote:
> Hi,
>
> Alvar Kusma found a bug in util/lar: If you try to add a file to a full
> LAR archive, the LAR utility will segfault. This is reproduced easily by
> zerofilling the LAR, then adding anything to it.
>
> Looking at the code, the reason is obvious:
> lar_empty_offset() can return an error code (-1). None of the callers
> check for an error code, they simply assume the return value is valid.
>
> Preliminary patch follows (and raises a few questions).
> Signed-off-by: Carl-Daniel Hailfinger
> Index: util/lar/lar.h
>
> --- util/lar/lar.h	(Revision 646)
> +++ util/lar/lar.h	(Arbeitskopie)
> @@ -58,7 +58,9 @@
>  #define BOOTBLOCK_NAME_LEN 16
>
> typedef uint64_t u64;
> +typedef int64_t s64;
>  typedef uint32_t u32;
> +typedef int32_t s32;
>  typedef uint8_t  u8;
>
> /* NOTE -- This and the coreboot lar.h may NOT be in sync. Be careful. */
> Index: util/lar/stream.c
> ===================================================================
> --- util/lar/stream.c	(Revision 646)
> +++ util/lar/stream.c	(Arbeitskopie)
> @@ -492,7 +492,7 @@
> @param lar the LAR archive structure
> @return The offset of the first chunk of empty space
>
> */
> -static int lar_empty_offset(struct lar *lar)
> +static s64 lar_empty_offset(struct lar *lar)
>  {
>    u32 offset = 0;
>    struct lar_header *header;
> @@ -508,10 +508,12 @@
>    	offset += get_next_offset(header);
>    }
> if (offset >= get_bootblock_offset(lar->size))
> if (offset >= get_bootblock_offset(lar->size)) {
> err("No empty space found!\n");	
> return -1;
> }
> return offset;
> return (s64)offset;
>
> }
>
> /**
> @@ -825,11 +827,16 @@
>  int maxsize(struct lar *lar, char *name)
>  {
>    int size;
> u32 offset;
>
> s64 offset;
> int bootblock_size;
>
> /* Find the beginning of the available space in the LAR */
>
> +#warning We should check all chunks of free space in the LAR. Right now we do not return the maximum size, but the size of the first chunk.
>    offset = lar_empty_offset(lar);
>
> if (offset < 0) {
> err("maxsize is negative\n");
> return offset;
>
> }
>
> /* Figure out how big our header will be */
> size = get_bootblock_offset(lar->size) - offset - header_len(name,NULL) - 1;
>
> @@ -878,7 +885,7 @@
>    int ret, hlen;
>    int pathlen;
>    u32 *walk,  csum;
> u32 offset;
>
> s64 offset;
>
> /* Find the beginning of the available space in the LAR */
> offset = lar_empty_offset(lar);
>
> @@ -886,7 +893,8 @@
>    /* Figure out how big our header will be */
>    hlen = header_len(pathname, &pathlen);
> if (offset + hlen + complen >= get_bootblock_offset(lar->size)) {
> if ((offset < 0) ||
> (offset + hlen + complen >= get_bootblock_offset(lar->size))) {
> err("Not enough room in the LAR to add the file.\n");
> return -1;
> }

[PATCH][RFC] LAR segfault when adding to a full archive

[Carl-Daniel Hailfinger]

On 28.03.2008 10:36, Stefan Reinauer wrote:
> NACK as long as the code adds #warnings. Please fix the issues and resubmit.

The warning has been added because this is a preexisting bug/limitation
in the code. Whenever I find a bug which is destined to explode in the
future, I add a #warning to it. That allows other people to fix the bug
and makes sure the bug is not forgotten.

What is our official policy for bugs we find, but which we don't have
time to fix?
- Forget about them?
- Add a FIXME comment?
- Add a #warning?

Regards,
Carl-Daniel
> Carl-Daniel Hailfinger wrote:
> > Index: util/lar/stream.c
> >
> > --- util/lar/stream.c	(Revision 646)
> > +++ util/lar/stream.c	(Arbeitskopie)
> > @@ -825,11 +827,16 @@
> >  int maxsize(struct lar *lar, char *name)
> >  {
> >    int size;
> > u32 offset;
> >
> > s64 offset;
> > int bootblock_size;
> >
> > /* Find the beginning of the available space in the LAR */
> >
> > +#warning We should check all chunks of free space in the LAR. Right now we do not return the maximum size, but the size of the first chunk.
> >    offset = lar_empty_offset(lar);
> >
> > if (offset < 0) {
> > err("maxsize is negative\n");
> > return offset;
> >
> > }
> >
> > /* Figure out how big our header will be */
> > size = get_bootblock_offset(lar->size) - offset - header_len(name,NULL) - 1;

[PATCH][RFC] LAR segfault when adding to a full archive

[Peter Stuge]

On Fri, Mar 28, 2008 at 11:39:44AM +0100, Carl-Daniel Hailfinger wrote:
> What is our official policy for bugs we find, but which we don't
> have time to fix?
> Forget about them?

No, this doesn't sound so good.
> Add a FIXME comment?
> Add a #warning?

Yeah, either of these is good I think.

The really ambitious adds tickets in trac, but I would probably just
add a #warning.

//Peter