Re: Review request for 5049299

On Fri, May 22, 2009 at 15:13, Michael McMahon wrote: > Martin, > > Thanks. Great comments. Just a few comments of my own > on a couple of points. > > 1. Linux won't benefit from this change as much as solaris, since due to > its > "memory overcommit" architecture, it doesn't suffer from the pro

Re: hg: jdk7/tl/jdk: 6843578: Re-implement IBM doublebyte charsets; ...

Am 22.05.2009 22:50, Xueming Shen schrieb: Ulf Zibis wrote: I think I have forgotten one thing: isisDoubleByte() in DoubleByte uses hard-wired values. Shouldn't they be better b1Min etc. ? These hardcoded values are explicitly specified by the "spec" of ebcdic doublebyte encoding. So I th

Re: Review request for 5049299

Martin, Thanks. Great comments. Just a few comments of my own on a couple of points. 1. Linux won't benefit from this change as much as solaris, since due to its "memory overcommit" architecture, it doesn't suffer from the problem (so much) in the first place (though memory overcommit ca

Re: Review request for 5049299

Meta: I have a special interest in this feature, partly because my employer has a special interest - but... only on Linux. I think glibc/linux has enough functionality to support cheap spawn as documented e.g. here: http://www.linuxfoundation.org/en/NewGlibc Martin

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley wrote: } if (mapLookup(locale_aliases, temp, &p)) { -strcpy(temp, p); +temp = realloc(temp, strlen(p)+1); +if (temp == NULL) { +JNU_ThrowOutOfMemoryError(env, NULL); +re

Re: Request for approval: fix 4428022, System.out.println(0.001) outputs 0.0010

Andrew Haley wrote: : OK, here it is. I put the test in java/lang/Double/ToString.java rather than sun/misc/FloatingDecimal because the bug manifested itself in Double.toString although the fix was in FloatingDecimal.dtoa. I agree that test/java/lang/Double is the best place for this. All loo

Re: hg: jdk7/tl/jdk: 6843578: Re-implement IBM doublebyte charsets; ...

Ulf Zibis wrote: I think I have forgotten one thing: isisDoubleByte() in DoubleByte uses hard-wired values. Shouldn't they be better b1Min etc. ? These hardcoded values are explicitly specified by the "spec" of ebcdic doublebyte encoding. So I think it's fine to leave them hard-wired. She

Re: Review request for 5049299

Hi Michael, I am very happy to see this being worked on; I would have done something like this already if I could fork() myself. This code is very tricky and has had many subtle bugs over the years. --- Any way we could add Linux support to this, via some flavor of low-level clone+exec? Let me

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley wrote: : Yes, you're right. :-( I reworked this silly patch so many times that I couldn't see it any more. As you say, encoding_variant needs to be at least the size of temp. Thanks for perseveringly with it. The latest patch looks good to me. -Alan.

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Alan Bateman wrote: > Andrew Haley wrote: >> : >> I've reworked the patch. >> >> I tried to refactor the code to make it cleaner and easier to follow, >> but I ended up touching so many places that it bacame a major change, >> and there's no way I could possibly test it all, especially the special

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley wrote: : I've reworked the patch. I tried to refactor the code to make it cleaner and easier to follow, but I ended up touching so many places that it bacame a major change, and there's no way I could possibly test it all, especially the special cases for Solaris. So, I backed off

Re: Request for approval: fix 4428022, System.out.println(0.001) outputs 0.0010

Alan Bateman wrote: > Andrew Haley wrote: >> This bug has been in Java for a long time, and the fix has been in >> IcedTea >> for a year and a half. >> >> The bug is a trivial off-by-one error. >> >> OK for OpenJDK7 and OpenJDK6? >> >> Andrew. >> > It looks reasonable to me although Joe would be

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Alan Bateman wrote: > Andrew Haley wrote: >> https://bugs.openjdk.java.net/show_bug.cgi?id=100057 >> >> GetJavaProperties has a stack-allocated fixed size buffer for holding >> a copy of >> a string returned by setlocale(3). However, there is no guarantee >> that the >> string will fit into this b

Re: Review request for 5049299

David Holmes - Sun Microsystems wrote: Hi Michael, > But the implementation in processhelper, does not know about JNI, so > it ignores the env and doesn't throw the exception, which then begs the question as to what happens if malloc returns NULL in that case? If I understand you right, the j

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

David Holmes - Sun Microsystems wrote: > If you use malloc then you have to check for a NULL return and deal with > the error possibility. > > Alternatively use strncpy to make sure it's safe and continue to assume > that it will be big enough. I'm working on fixing this properly, but I just cam

Re: Review request for 5049299

Michael McMahon wrote: Hi, I have just posted a webrev for 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion. I'm very happy to see this one and I will review it in detail over the next few days. I'm sure Martin will want to review it too as he nurtured that code f

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

Doug Lea said the following on 05/22/09 21:56: Sorry; I should have been clearer about why c.toArray(new Object[c.size()]) is subtly wrong here. ArrayList.size must equal the number of elements, which might be different than the array size. If c's size shrinks at an inconvenient moment during t

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley wrote: : OK. I'll check for the NULL, then. If I have to change the patch that's been in IcedTea for ages then I'll use strdup instead of malloc. But what is one supposed to do if the allocation fails? Simply emit an error message to stderr and call abort() ? JNU_ThrowOutOfMem

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley said the following on 05/22/09 21:45: David Holmes - Sun Microsystems wrote: If you use malloc then you have to check for a NULL return and deal with the error possibility. Alternatively use strncpy to make sure it's safe and continue to assume that it will be big enough. It's j

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

David Holmes - Sun Microsystems wrote: Doug Lea said the following on 05/22/09 21:31: David Holmes - Sun Microsystems wrote: Thanks for the info, one query though ... Ummm why didn't it just use: elementData = c.toArray(new Object[c.size()]); Because "c" might be a concurrent collectio

Re: Request for approval: fix 4428022, System.out.println(0.001) outputs 0.0010

Andrew Haley wrote: This bug has been in Java for a long time, and the fix has been in IcedTea for a year and a half. The bug is a trivial off-by-one error. OK for OpenJDK7 and OpenJDK6? Andrew. It looks reasonable to me although Joe would be the best person to review this. It would be goo

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

David Holmes - Sun Microsystems wrote: > If you use malloc then you have to check for a NULL return and deal with > the error possibility. > > Alternatively use strncpy to make sure it's safe and continue to assume > that it will be big enough. It's just following the style used throughout that

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

Doug Lea said the following on 05/22/09 21:31: David Holmes - Sun Microsystems wrote: Thanks for the info, one query though ... Ummm why didn't it just use: elementData = c.toArray(new Object[c.size()]); Because "c" might be a concurrent collection, so you don't want to independently ca

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Andrew Haley wrote: https://bugs.openjdk.java.net/show_bug.cgi?id=100057 GetJavaProperties has a stack-allocated fixed size buffer for holding a copy of a string returned by setlocale(3). However, there is no guarantee that the string will fit into this buffer. This one is probably due to Sola

Re: Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

Hi Andrew, If you use malloc then you have to check for a NULL return and deal with the error possibility. Alternatively use strncpy to make sure it's safe and continue to assume that it will be big enough. Cheers, David Holmes Andrew Haley said the following on 05/22/09 21:10: https://bu

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

David Holmes - Sun Microsystems wrote: Thanks for the info, one query though ... Ummm why didn't it just use: elementData = c.toArray(new Object[c.size()]); Because "c" might be a concurrent collection, so you don't want to independently call c.size(). Notice that AbstractCollection corr

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

Hi Doug, Thanks for the info, one query though ... Doug Lea said the following on 05/22/09 21:08: David Holmes - Sun Microsystems wrote: Okay well the bug is still open. I think the original intent was to change toArray() to match this, but I think it's far too late to change that behaviour n

Re: Review request for 5049299

Hi Michael, > But the implementation in processhelper, does not know about JNI, so > it ignores the env and doesn't throw the exception, which then begs the question as to what happens if malloc returns NULL in that case? If I understand you right, the jlup_* functions called from processhelpe

Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

https://bugs.openjdk.java.net/show_bug.cgi?id=100057 GetJavaProperties has a stack-allocated fixed size buffer for holding a copy of a string returned by setlocale(3). However, there is no guarantee that the string will fit into this buffer. This one is probably due to Solaris code being reused

Re: c.toArray might (incorrectly) not return Object[] (see 6260652)

David Holmes - Sun Microsystems wrote: Florian Weimer said the following on 05/22/09 14:46: It's there, but not in the documentation for toArray(): | Note that toArray(new Object[0]) is identical in function to toArray().

Re: Review request for 5049299

Hi David, In the Makefile: + HELPER_EXE = $(BINDIR)/processhelper$(EXE_SUFFIX) Isn't EXE_SUFFIX superfluous here? It has to be an empty string otherwise the Java code won't know the name of the helper. Yes, it is superfluous. I took the pattern from another makefile, but it is probably be

Re: Review request for 5049299

Hi Michael, I can't say that I grok all the detail in this but I get the gist and overall I don't see anything obviously problematic. I have a couple of small comments: In the Makefile: + HELPER_EXE = $(BINDIR)/processhelper$(EXE_SUFFIX) Isn't EXE_SUFFIX superfluous here? It has to be an em

Request for approval: fix 4428022, System.out.println(0.001) outputs 0.0010

This bug has been in Java for a long time, and the fix has been in IcedTea for a year and a half. The bug is a trivial off-by-one error. OK for OpenJDK7 and OpenJDK6? Andrew. --- oipenjdk/jdk/src/share/classes/sun/misc/FloatingDecimal.javaThu Sep 27 12:56:46 2007 -0700 +++ openjdk/jdk

Re: hg: jdk7/tl/jdk: 6843578: Re-implement IBM doublebyte charsets; ...

Very BIG THING ! ... my browser nearly collapsed. I think I have forgotten one thing: isisDoubleByte() in DoubleByte uses hard-wired values. Shouldn't they be better b1Min etc. ? -Ulf Am 22.05.2009 08:56, xueming.s...@sun.com schrieb: Changeset: 914c33c7de3e Author:sherman Date:

Review request for 5049299

Hi, I have just posted a webrev for 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion. webrev location: http://cr.openjdk.java.net/~michaelm/5049299/webrev.00/ **I'd like to give an outline of the change here, to make reviewing the webrev a bit easier. Basically,