On 29/09/2009, Oliver Deakin <oliver.dea...@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 > >
