Re: [coreboot] [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;
> }