[PATCH FOR REVIEW] Potential Buffer Overflow in java_props_md.c

[Andrew Hughes]

----- Original Message -----
> Hi Andrew,
> 
> On 2/08/2012 7:35 PM, Andrew Hughes wrote:
> > ----- Original Message -----
> >> Andrew et al,
> >>
> >> AFAICS here:
> >>
> >>         220     encoding_variant = malloc(strlen(temp)+1);
> >>         221     if (encoding_variant == NULL) {
> >>         222         JNU_ThrowOutOfMemoryError(env, NULL);
> >>         223         return 0;
> >>         224     }
> >>
> >> we also need to do free(temp). Similarly later where we return
> >> with
> >> OOM
> >> due to realloc failure, don't we also need to free what was
> >> previously
> >> malloc'd?
> >>
> >
> > I was thinking the same just before committing, but didn't want to
> > delay
> > the main fix any further.
> >
> > While logically we do need one, I'm not sure it's worthwhile if
> > we're going
> > to throw the exception and exit anyway?  Will the return even be
> > reached?
> > If we're already near enough to OOM that we can't allocate more
> > memory,
> > it's unlikely freeing temp is going to get us much, and I presume
> > it will
> > be freed anyway when the VM process exists.
> 
> Are we definitely going to exit on this error? I agree if we're out
> of
> memory we're likely to continue to run into problems if we try to
> continue. But I'd prefer to see proper cleanup rather than making
> assumptions about the context in which the code is used.
> 
> > But I can add it if necessary.  It's trivial after all and does not
> > real
> > harm.
> 
> It will also prevent us getting flagged with memory-leak
> warnings/errors
> from static analysis tools.
> 

This should cover it, I think:

http://cr.openjdk.java.net/~andrew/buffer_overflow/webrev.02/

but it'll need a bug ID.

> Thanks,
> David
> 
> >> David
> >>
> >> On 2/08/2012 7:18 AM, Andrew Hughes wrote:
> >>>
> >>>
> >>> ----- Original Message -----
> >>>> On 01/08/2012 14:52, Andrew Hughes wrote:
> >>>>> :
> >>>>>
> >>>>>
> >>>>> In any case, there is a Sun bug open for this:
> >>>>>
> >>>>> 6844255: Potential stack corruption in GetJavaProperties
> >>>>>
> >>>>> Can I take it that I can just get on and push Omair's extended
> >>>>> version now then,
> >>>>> with that bug ID?
> >>>> Yes, go ahead, I should have said that in my mail.
> >>>>
> >>>
> >>> Thanks.
> >>>
> >>> Done:
> >>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-August/010993.html
> >>>
> >>> with Omair as author and yourself and I as reviewers.
> >>>
> >>>>> Well, the locale can be set be an environment variable, so it
> >>>>> could
> >>>>> potentially
> >>>>> be anything of any length...
> >>>>>
> >>>>> The Debian bug posted above has an example, though I couldn't
> >>>>> replicate it.
> >>>>>
> >>>> I couldn't replicate it either and was just curious if anyone
> >>>> managed
> >>>> to
> >>>> demonstrate it.
> >>>>
> >>>
> >>> Yeah, I tend to think it's more potentially exploitable rather
> >>> than
> >>> something
> >>> that's actually been hit.
> >>>
> >>>> -Alan.
> >>>>
> >>>
> >>> Thanks,
> >>
> >
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07