[uClinux-dev] [PATCH] : Avoid filename < TASK_SIZE test in do_getname() when no MMU

Jamie Lokier jamie at shareable.org
Mon May 24 19:20:47 EDT 2010


Philippe De Muyter wrote:
> On Mon, May 24, 2010 at 06:26:08PM +0100, Jamie Lokier wrote:
> > Philippe De Muyter wrote:
> > > On Mon, May 24, 2010 at 04:59:18PM +0100, David Howells wrote:
> > > > Philippe De Muyter <phdm at macqel.be> wrote:
> > > > 
> > > > > > +#else
> > > > > > +#define TASK_SIZE	(0xFFFFFFFFUL)
> > > > > > +#endif
> > > > > 
> > > > > Because of do_getname() :
> > > > > 
> > > > > 	len = TASK_SIZE - (unsigned long) filename;
> > > > > 
> > > > > we should rather have
> > > > > 
> > > > > 	#define TASK_SIZE (0x100000000ull)
> > > > 
> > > > Do you guarantee that will work everywhere on a 32-bit system, though?
> > > > 
> > > > Note that it also makes things slower as gcc has to start using 64-bit
> > > > arithmetic where it could otherwise use 32-bit arithmetic.
> > > 
> > > Except if gcc notices that this simplifies to
> > > 
> > > 	len = (unsigned long)(-filename);
> > > 
> > > I don't know if it does.
> > 
> > It did simplify on the x86 (gcc 4.4) and ARM (gcc 3.4.3) tests I just did.
> > Also the comparison "addr < TASK_SIZE" simplified to "1".
> > 
> > However I can imagine some logic like this going awry with that
> > definition:
> > 
> >     if (addr >= TASK_SIZE || len > TASK_SIZE - addr)
> > 	return -EINVAL;
> > 
> > Think about the case addr == 0.
> 
> It would give ( 0 || len > (unsigned long)(-addr))
> I don't see a problem there.

The problem is it would return -EINVAL for any non-zero value of len,
which isn't what the code looks like it's meant to do.

(By luck it would be ok because rejecting NULL addr is probably good
anyway.  But that might not be true for every check of that form.)

-- Jamie



More information about the uClinux-dev mailing list