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

Xueming Shen xueming.shen at oracle.com
Wed Aug 8 01:13:32 UTC 2012

On 8/7/12 5:33 PM, David Holmes wrote:

On 8/08/2012 2:40 AM, Xueming Shen wrote:

Andrew, Since we are here:-) are we also supposed to "free" the oldtemp at #250 and oldtemp and oldev at the end? No. They are aliases for temp and encodingvariant, which are freed at the end. There are only ever at most two things to free.

oops, I did not see it's a realloc....

Andrew: this is fine by me, but needs TL approval (Alan?) David -----

-Sherman On 08/07/2012 04:49 AM, Andrew Hughes wrote: ----- 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,

More information about the core-libs-dev mailing list