harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [classlib][luni] calling mmap
Date Tue, 29 Sep 2009 16:09:51 GMT
On 29/09/2009, Oliver Deakin <oliver.deakin@googlemail.com> wrote:
> Tim Ellison wrote:
>
> > On 29/Sep/2009 15:40, Oliver Deakin wrote:
> >
> >
> > > Tim Ellison wrote:
> > >
> > >
> > > > Making size == 0 a special case that always succeeds (even if it is
> > > > returning a bogus address) bypasses all the other checks that the mmap
> > > > function call would have made.
> > > >
> > > > For example, what if the file descriptor was invalid? or points to an
> > > > unmappable file?  Then I would expect an IOException.
> > > >
> > > >
> > > I think (from the JIRA) that we were getting EINVAL back from the mmap
> > > call with size 0, and that's why it was fixed before making the mmap
> > > call. It would be good to know what the RI does in the case where size
> > > is 0 and we're also mapping an invalid file - do we get an exception
> > > thrown? Perhaps Catherine can clarify?
> > >
> > >
> >
> > Ok, I missed that.  Then perhaps the special case should be on
> > PlatformAddressFactory#allocMap() so that we don't invoke
> the overhead
> > of managing the 'bogus' 0 as if it were a real address that needs
> > freeing etc.
> >
> >
> >
>
>  Ok, that sounds reasonable - Ill take a look.
>
>
> >
> > >
> > > > There is nothing in the man pages to say that size = 0 is invalid.
> > > >
> > > >
> > > No, but in the JIRA it indicates that we receive an EINVAL return code
> > > when passing in size 0, so perhaps it is just not in the man page. The
> > > man page I am looking at also didn't explicitly say negative values are
> > > illegal, but I think we can assume that's true.
> > >
> > >
> >
> > The man pages I'm looking at declare size as a (size_t) which is usually
> > unsigned ;-)
> >
> >
>
>  haha, ok that's true ;)
>
>
> >
> >
> > >
> > > > Plus, there is now no path that will return -1 as the address, so you
> > > > can remove the check (all the exceptions are raised in the native
> code).
> > > >
> > > >
> > > That's true, I hadn't spotted that. I also noticed a bug in the native
> > > code change where we allocate 100 chars for the error string but the
> > > EAGAIN error message is 101 characters long (plus another 1 for the null
> > > terminator) so we're off by 2 there.

Surely it's unsafe to use *any* string manipulation routines without
checking that the target buffer has enough space?

Such buffer overflows are often exploitable to cause application
crashes or worse.

> > > Perhaps a neater way to get these
> > > error messages would be to use strerror() once we have the error number,
> > > but for now Ill fix this array size bug and remove the redundant -1
> > > check in the java code for now, and we can keep discussing the correct
> > > behaviour here.
> > >
> > >
> >
> > I don't know what the correct behavior is in this case, I just mentioned
> > it because it looked odd.
> >
> >
>
>  Understood - I think matching the RI here makes sense. I committed the
> removal of the -1 check and the char array size bug at repo revision
> r819983.
>
>  Regards,
>  Oliver
>
>
> > Regards,
> > Tim
> >
> >
> >
>
>  --
>  Oliver Deakin
>  Unless stated otherwise above:
>  IBM United Kingdom Limited - Registered in England and Wales with number
> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6 3AU
>
>

Mime
View raw message