[coreboot] lar directory handling patch
Myles Watson
mylesgw at gmail.com
Tue Feb 26 01:00:41 CET 2008
> -----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
More information about the coreboot
mailing list