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

[Andrew Hughes]

----- Original Message -----
> Thanks Andrew. 7189533 created for this.
> 

Thanks.  Ok to push then? :-)

> David
> -----
> 
> On 3/08/2012 8:03 AM, Andrew Hughes wrote:
> >
> >
> > ----- 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