[coreboot] lar directory handling patch

[Myles Watson]

> -----Original Message-----
> From: coreboot-bounces at coreboot.org [mailto:coreboot-bounces at coreboot.org]
> On Behalf Of Peter Stuge
> Sent: Monday, February 25, 2008 4:34 PM
> To: Coreboot
> Subject: Re: [coreboot] lar directory handling patch
> 
> On Mon, Feb 25, 2008 at 01:47:59PM -0700, Myles Watson wrote:
> >  				strncpy(fullname, name, MAX_PATH);
> > +				strncpy(fullpathname, pathname, MAX_PATH);
> 
> --8<-- strcpy(3)
> The strncpy() function is similar, except that at most n bytes  of  src
> are  copied.  Warning: If there is no null byte among the first n bytes
> of src, the string placed in dest will not be null terminated.
> -->8--
> 
> One fix is to simply memset() fullname and fullpathname at the start
> of the function - I don't like assuming that heap variables are
> zero initialized.

I guess I figured that the chance of having pathnames > 1024 was pretty
close to zero, so I didn't change the way it was done.  I guess I could just
set the last byte of the array to '\0' and only strncpy MAX_PATH-1 too.

I'll use memset.

> 
> >  				if (name[(strlen(name)) - 1] != '/') {
> >  					strncat(fullname, "/", MAX_PATH);
> >  				}
> >
> > +				if (name[(strlen(pathname)) - 1] != '/') {
> > +					strncat(fullpathname, "/",
MAX_PATH);
> > +				}
> > +
> >  				strncat(fullname, namelist[n]->d_name,
> >  					MAX_PATH);
> >
> > -				add_files(fullname);
> > +				strncat(fullpathname, namelist[n]->d_name,
> > +					MAX_PATH);
> > +
> > +				add_files(fullname,fullpathname,thisalgo);
> 
> This algorithm protects against overflow, but I would like if it also
> raised an error when MAX_PATH isn't big enough.

Sure, I can add that.  I guess I should be more careful about the math for
the strncat too.  It looks like I've left open the possibility of adding two
paths < MAX_PATH and getting one larger than MAX_PATH.  Again I guess we're
hoping that no one will ever use really long directory names and deep
nesting on us.

> 
> > +int add_files(const char *name, const char * pathname_in,
> > +		const enum compalgo algo_in)
> ..
> > +		ret = lar_process_name((char*)name, &filename, &pathname,
> &thisalgo);
> 
> Why is this cast needed? Does lar_process_name() modify name? If not
> please fix it's prototype so no cast is needed.

It does.  It changes ':' to '\0' for files like:

nocompress:payload-filo.elf:normal/payload

 
> 
> > +	/*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/
> 
> Is there some debug functionality in lar for these?

Not that I saw.  You're talking about a verbose switch or something like
that?

> >  	if (elfparse() && iself(ptr)) {
> > -		output_elf_segments(lar, pathname, ptr, size, thisalgo);
> > +		output_elf_segments(lar, file->pathname, ptr, size, file-
> >algo);
> 
> How does this work with file->algo vs. zeroes compression?

It doesn't change it.  The .bss segment is treated specially, and overrides
the compression mode.

> 
> 
> //Peter

Thanks for reviewing,

Myles