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

Jamie Lokier jamie at shareable.org
Tue May 25 13:05:00 EDT 2010


Philippe De Muyter wrote:
> On Tue, May 25, 2010 at 12:20:47AM +0100, Jamie Lokier wrote:
> > 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))
> 
> Sorry.  I should not have answered that late.
> Of course, for (addr == 0), it would give
> 
> 		( 0 || len > TASK_SIZE)
> 
> and hence
> 		( 0 || 0)
> 
> > > 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.
> 
> No, see above.
> 
> Of course, if one computes the maximum len (TASK_SIZE - addr) separately
> and stores it in a unsigned 32bit, then there is a design error in the
> program.

You're right.  I should not have answered that late either ;-)

-- Jamie



More information about the uClinux-dev mailing list