[LinuxBIOS] [PATCH][LAR] New LAR access functions

Stefan Reinauer stepan at coresystems.de
Thu Jul 12 16:49:35 CEST 2007


* Peter Stuge <peter at stuge.se> [070712 00:49]:
> > +static int lar_read_size(struct lar *lar, int filelen)
> > +{
> > +	unsigned int *ptr = (unsigned int *) (lar->map + (filelen - 12));
> > +	return ptr[0];
> > +}
> 
> What about all this pointer arithmetic? Is it really correct?
 
This code is not new, it is in another file already.

> Doesn't adding 1 to a pointer actually add sizeof pointed-to-type to
> the address? Ie. this code isn't portable?

What makes you think so? It fetches an int from 12 bytes before the end
of the file (ie. after the reset vector). 

It is of course not portable because there is no reset vector at
fffffff0 on non x86 machines. But that's another issue.

> Then there's the matter of the pointer arithmetic again. Since ptr is
> uchar * this will work, but then writing size to p[0] will write a
> different number of bytes on different archs, right?

int is 32bit on all GNU architectures I know that could theoretically
run LinuxBIOS. But since this is an x86 only thing anyways I would not
care too much.
 
> > +	lar =  _open_lar(archive, s.st_size, O_RDWR);
> 
> Race conditions. First open() then fstat() to get the size.
> 
> Also, the file size can change even though we've opened the file.
> 
> Since the size is a rather important variable throughout lar we
> want to be able to handle it changing under our feet, but I don't
> know what the best way would be?
 
What kind of situation that realisticly happens would produce such an
issue? i.e. While a payload builds a lar, or while LinuxBIOS is
compiled?

> > +	mkdirp((const char *) dirname(path), 0755);
> > +	free(path);
> > +
> > +	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> > +
> > +	if (fd == -1) {
> > +		err("Error creating file %s\n", filename);
> > +		return -1;
> > +	}
> 
> I don't think lar should mkdir -p implicitly. Just open() and
> perror() or print strerror(errno) in case of failure.
 
Oh it does not do this implicitly. It only does it if the archive
explicitly contains a directory.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.dehttp://www.coresystems.de/




More information about the coreboot mailing list