[PATCH FOR REVIEW] Potential Buffer Overflow in java_props_md.c (original) (raw)

Andrew Hughes ahughes at redhat.com
Tue Aug 7 11:49:28 UTC 2012

----- 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 encodingvariant = malloc(strlen(temp)+1); >>>> 221 if (encodingvariant == NULL) { >>>> 222 JNUThrowOutOfMemoryError(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/bufferoverflow/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

More information about the core-libs-dev mailing list