In message <25aac9fc0909290909v2e012fdfnc93e37eb32c79...@mail.gmail.com>,
sebb writes:
>
> On 29/09/2009, Oliver Deakin <oliver.dea...@googlemail.com> wrote:
> > Tim Ellison wrote:
> >
> > > On 29/Sep/2009 15:40, Oliver Deakin wrote:
> > >
[SNIP]
> > >
> > > > 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?
Well, no. It is unsafe if you don't "control" all of the inputs. In
this case, we do since most of the inputs are just strcpy'd static
strings. The other two are contain '%d' format strings that I'm pretty
sure can't overflow the buffer.
I'm really puzzled why no one else has asked ... why are we copying
static strings? I must be missing something?
(They are "const char*" static strings and only seem to be used in a
"const char*" context so it makes no sense to copy them to a "char*".)
I also don't understand why the EACCESS string doesn't contain "with
error EACCES" so that it is consistent with the other cases. But this
is largely irrelevant since I don't think these strings add value
anyway... see below.
> Such buffer overflows are often exploitable to cause application
> crashes or worse.
Indeed. (This reminds me I must fix the one confusing one I found
in addDirsToPath in the launcher main.c. From memory, if the first
newPathToAdd is found then strLen is short by one separator because the
logic for calculating the length and doing the concatenation is not
consistent.)
> > > > Perhaps a neater way to get these error messages would be to use
> > > > strerror() once we have the error number,
I agree. Creating these contrived error messages is almost certainly
not productive since people are used to seeing the familiar
strerror/perror error messages. I don't see why we'd choose to create
custom error strings here and not do the same for all the other syscalls
in the classlib natives.
strerror isn't thread-safe so we probably shouldn't use this but rather
the XSI strerror_r. However, we seem to be making this more complicated
than it needs to be. Why not just something simple like:
PORT_ACCESS_FROM_ENV(env);
...
if(mapAddress == MAP_FAILED) {
hyerror_set_last_error(errno, HYPORT_ERROR_OPFAILED);
throwJavaIoIOException(env, hyerror_last_error_message());
return -1;
}
and let portlib do the work? We could make it more complicated and use
another buffer/sprintf/etc to add a prefix but I think the "Call to mmap
failed with error EBLAH" is rather redundant since the stacktrace from
the exception will clearly show the call to mmapImpl anyway.
I think the switch statement is just bloat and doesn't really add
anything apart from some unnecessary complexity and far more code to
read than is required to get the job done.
That's enough ranting for now. ;-)
Regards,
-Mark.
