[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 <c-d.hailfinger.devel.2006 at gmx.net>
>
> 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;
>  	}
>
>
>
>