----- Original Message -----

On 01/08/2012 12:40, Andrew Hughes wrote:

...

java_props_md.c allocates a 64 byte buffer for the return value of setlocale on the stack. However, there appears to be no set limit on the return value:

http://pubs.opengroup.org/onlinepubs/009604499/functions/setlocale.html

and no check in the code to ensure that its length is 63 characters or less (allowing for '\0'). While language and country are defined by ISO, I don't believe there's any limit on the optional encoding and variant entries.

This patch:

http://cr.openjdk.java.net/~andrew/buffer_overflow/webrev.01/jdk.patch

replaces the allocation with a dynamic buffer based on the length of lc.

Original bug: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=497666

Ok for tl? If so, can I have a bug ID?

Just some history on this one.

Andrew Haley originally brought this up up in 2009 (embarrassing, I know), see:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/001650.html

Then in 2011, Omair tried again, see:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-May/006907.html

Well, it's a Red Hat patch and we're all Red Hat employees :-) The original version, posted by Andrew Haley and in IcedTea, was actually written by Lillian Angel back in 2008: 2008-09-10 Lillian Angel <langel@redhat.com> * patches/icedtea-lc_ctype.patch: New patch to fix this issue: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=497666 * Makefile.am: Added patch to list. 2008-09-15 Lillian Angel <langel@redhat.com> * patches/icedtea-lc_ctype.patch: Fixed array size and changed to use malloc/free. and she's since left Red Hat. My version is based on this original, the one in IcedTea, with the addition of a check on the return value of malloc. I have no idea what happened with Omair's extended version. It's not in IcedTea.

I reviewed it at the time and gave a thumbs up, it's just it was the end-game for 7 at the time and that release was being locked down. I asked that it be held back until jdk8 and jdk7u.

Omair's webrev is a larger patch that what you are proposing now, mostly handling malloc/realloc failing that we nit-picked in the original review. I haven't checked IcedTea to know whether it has the minimal patch you are proposing now or whether it has Omair's changes.

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?

Also if you read the old mails then you'll see that we were scratching our heads as to an example that would demonstrate the original issue. I suspect it may have been something that someone spotted rather than someone running with a locale of this length.

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.

-Alan.

-- 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

----- Original Message -----

On 08/01/2012 09:52 AM, Andrew Hughes wrote:

...

I have no idea what happened with Omair's extended version. It's not in IcedTea.

I didn't commit it to icedtea since I assumed I would be committing it to OpenJDK7/8 anyway (and icedtea would get it on the next sync). And I didn't commit it to OpenJDK7 because only critical fixes were being accepted. Then I guess this patch fell off my radar when 7u/8 forests were created.

Yes, I can see the reasoning, but in retrospect, it seems a good idea to add patches to IcedTea straight away so they don't get lost, should review take a while for some reason (and they have to go through two to reach 6 or 7). It's trivial to merge the two when they do come in.

...

Can I take it that I can just get on and push Omair's extended version now then, with that bug ID?

Definitely no objections from me.

Done: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-August/010993.html

Cheers, Omair

-- 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

----- 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

----- 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. But I can add it if necessary. It's trivial after all and does not real harm.

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

----- 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

----- 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

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

Andrew,

Since we are here:-) are we also supposed to "free" the old_temp at #250 and old_temp and old_ev at the end?

No. They are aliases for temp and encoding_variant, which are freed at the end. There are only ever at most two things to free. 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 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,

On 08/08/2012 01:33, 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 old_temp at #250 and old_temp and old_ev at the end?

No. They are aliases for temp and encoding_variant, which are freed at the end. There are only ever at most two things to free.

Andrew: this is fine by me, but needs TL approval (Alan?) You are a reviewer on the jdk8 project so Andrew should be all set. In any case, the update looks good to me too.

-Alan.

On 02/08/2012 03:14, David Holmes wrote:

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?

David I see there are follow-on mails to this but just to say that this is System.initProperties time so if there is a malloc failure this early in startup then it will cause the VM initialization to fail. So for completeness (and perhaps native memory leak detection tools) then I agree but if we do have problems here then we aren't go to go very far.

-Alan.