Re: JDK 9 RFR of 8064463: BigDecimal should populate NumberFormatException message
On 22/12/2014 21:13, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8064463 Patch: http://cr.openjdk.java.net/~bpb/8064463/webrev.00/ Summary: Ensure that the message strings of all NumberFormatExceptions are populated with appropriate content, excluding where the NFE merely wraps another exception in its cause. It was suggested in the issue description to include the contents of the supplied character array in the exception message. This is not done in this patch for the reason that the resulting string could be quite large. If it seems reasonable to include the characters the patch can be so amended. This looks okay to me. A minor inconsistency is that one of the Exponent overflow cases has a trailing dot, the other doesn't. -Alan
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 24/12/2014 11:37, Peter Levart wrote: Hi Brad, Thanks for looking into this. Here's updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/ This mostly looks good to me too, except the permission checking. As I read it, getInputStream uses getCanonicalFile and thus the permission check will be happen early and so it makes me wonder if checkRead is needed. Additionally, both of the uses are in privileged blocks so it looks like checkRead will always be called with a stack that has AllPermission anyway. -Alan.
Re: RFR: 8066834: tools/pack200/CommandLineTests.java does not conform ProblemList.txt style
On 24/12/2014 18:56, Kumar Srinivasan wrote: Hello, Please review this simple fix, enclosed. Thanks in advance. Kumar diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -267,7 +267,7 @@ # Tests take too long, on sparcs see 7143279 # also see 8059906 -tools/pack200/CommandLineTests.java +tools/pack200/CommandLineTests.java generic-all tools/pack200/Pack200Test.java solaris-all,macosx-all I don't know this slipped in but the change looks okay to me. -Alan.
Re: RFR: 8067889: 4 pack200 tests fail on mac since jdk became modular
On 26/12/2014 17:42, Kumar Srinivasan wrote: New webrev uploaded with comment suggested by Amy Lu. The revised patch is here: http://cr.openjdk.java.net/~ksrini/8067889/webrev.1/webrev.delta/index.html The full patch: http://cr.openjdk.java.net/~ksrini/8067889/webrev.1 Updated patch looks good to me. -Alan.
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 12/29/2014 10:08 AM, Alan Bateman wrote: On 24/12/2014 11:37, Peter Levart wrote: Hi Brad, Thanks for looking into this. Here's updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/ This mostly looks good to me too, except the permission checking. As I read it, getInputStream uses getCanonicalFile and thus the permission check will be happen early and so it makes me wonder if checkRead is needed. Additionally, both of the uses are in privileged blocks so it looks like checkRead will always be called with a stack that has AllPermission anyway. -Alan. Thanks for looking at this, Alan. You're right about File.getCanonicalFile(). It already checks read permission for a file. The additional explicit check is superfluous. I have removed it. With explicit check I wanted the API to behave uniformly regardless of whether the returned stream is opened by getInputStream() call or an already opened stream is just returned. getCannonicalFile() already takes care of it. Here's the updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ Regards, Peter
Re: RFR: 8068230: java.util.zip.Deflater.init throws InternalError
Thanks Sherman and Alan for the reference to JDK-8008759! It does seem to be the root cause of this issue also. I've just requested an approval to backport that fix into jdk 7u. To improve the error message of Deflater.init, I've created another issue: JDK-8068338. Sincerely yours, Ivan On 28.12.2014 18:04, Alan Bateman wrote: On 26/12/2014 08:58, Ivan Gerasimov wrote: Hello! Some zlib-related tests fail on Solaris hosts, throwing InternalError with no detailed message. This might be due to incompatible version of zlib library installed on the host. To have a better diagnostics, it is proposed to specifically check for Z_VERSION_ERROR error code and issue a specific message in that case. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8068230 WEBREV: http://cr.openjdk.java.net/~igerasim/8068230/0/webrev/ There isn't sufficient information in the bug report but since it is a fastdebug build then it would be good to get JDK-8008759 back-ported to 7uX and see if the issue duplicates. As regards the proposed change to improve then exception when an incompatible zlib is encountered then it probably should be moved to another issue. One suggestion is to keep it as an internal error and use the same exception message as is already done in Inflater.init. -Alan.
RFR: 8068338: Better message about incompatible zlib in Deflater.init
Hello! It it proposed to enhance the error message in the manner it was done in JDK-8008759. Would you please help review this simple change? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8068338 WEBREV: http://cr.openjdk.java.net/~igerasim/8068338/0/webrev/ Sincerely yours, Ivan
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 29/12/2014 09:45, Peter Levart wrote: Thanks for looking at this, Alan. You're right about File.getCanonicalFile(). It already checks read permission for a file. The additional explicit check is superfluous. I have removed it. With explicit check I wanted the API to behave uniformly regardless of whether the returned stream is opened by getInputStream() call or an already opened stream is just returned. getCannonicalFile() already takes care of it. Here's the updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ Updated patch looks good to me. -Alan.
Re: RFR: 8068338: Better message about incompatible zlib in Deflater.init
On 29/12/2014 11:40, Ivan Gerasimov wrote: Hello! It it proposed to enhance the error message in the manner it was done in JDK-8008759. Would you please help review this simple change? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8068338 WEBREV: http://cr.openjdk.java.net/~igerasim/8068338/0/webrev/ This looks okay to me. I assume that JDK-8068230 will be closed once it is verified that JDK-8008759 is the real issue. -Alan.
Re: JDK 9 RFR of 8064463: BigDecimal should populate NumberFormatException message
On Dec 29, 2014, at 12:46 AM, Alan Bateman alan.bate...@oracle.com wrote: Patch: http://cr.openjdk.java.net/~bpb/8064463/webrev.00/ […] This looks okay to me. A minor inconsistency is that one of the Exponent overflow cases has a trailing dot, the other doesn't. Thanks. I’ll add a dot at line 559 before pushing. Brian
Re: Explicit Serialization API and Security
So, if I understand this correctly, the way this would get used is:
class BaseFoo implements Serializable {
private final int x;
public BaseFoo(ReadSerial rs) {
this(rs.getInt(x));
}
public BaseFoo(int x) {
this.x = x;
}
}
Right?
What happens with subclasses? I think then I need an extra RS arg in my
constructors:
class SubFoo extends BaseFoo {
private final int y;
public SubFoo(ReadSerial rs) {
this(rs.getInt(y));
}
public BaseFoo(ReadSerial rs, int y) {
super(rs);
this.y = y;
}
}
Is this what you envision?
On 12/27/2014 8:03 PM, Peter Firmstone wrote:
Is there any interest in developing an explicit API for Serialization?:
1. Use a public constructor signature with a single argument,
ReadSerialParameters (read only, writable only by the
serialization framework) to recreate objects, subclasses (when
permitted) call this first from their own constructor, they have
an identical constructor signature. ReadSerialParameters that are
null may contain a circular reference and will be available after
construction, see #3 below.
2. Use a factory method (defined by an interface) with one parameter,
WriteSerialParameters (write only, readable only by the
serialization framework), this method can be overridden by
subclasses (when permitted)
3. For circular links, a public method (defined by an interface) that
accepts one argument, ReadSerialParameters, this method is called
after the constructor completes, subclasses overriding this should
call the superclass method. If this method is not called, an
implementation, if known to possibly contain circular links,
should check it has been fully initialized in each object method
called.
4. Retains compatibility with current serialization stream format.
5. Each serial field has a name, calling class and object reference,
similar to explicitly declaring private static final
ObjectStreamField[] serialPersistentFields .
Benefits:
1. An object's internal form is not publicised.
2. Each class in an object's heirarchy can use a static method to
check invarients and throw an exception, prior to
java.lang.Object's constructor being called, preventing
construction and avoiding finalizer attacks.
3. Final field friendly.
4. Compatible with existing serial form.
5. Flexible serial form evolution.
6. All methods are public and explicitly defined.
7. All class ProtectionDomain's exist in the current execution
context, allowing an object to throw a SecurityException before
construction.
8. Less susceptible to deserialization attacks.
Problems:
1. Implementations cannot be package private or private. Implicit
serialization publicises internal form, any thoughts?
Recommendations:
1. Create a security check in the serialization framework for
implicit serialization, allowing administrators to reduce their
deserialization attack surface.
2. For improved security, disallow classes implementing explicit
serialization from having static state and static initializer
blocks, only allow static methods, this would require complier and
verifier changes.
3. Alternative to #2, allow final static fields, but don't allow
static initializer blocks or mutable static fields, similar to
interfaces.
Penny for your thoughts?
Regards,
Peter Firmstone.
RFR: 8068347: Add java/lang/ClassLoader/deadlock/GetResource.java to ProblemList.txt
Hi,
The
java/lang/ClassLoader/deadlock/GetResource.java
test has started causing problems for Hotspot nightly testing. A real
fix is being worked on under [1], but in the meantime, this test should
be added to the ProblemList.
Bug:
https://bugs.openjdk.java.net/browse/JDK-8068347
The diff ('jdk' repo) is:
--- a/test/ProblemList.txt Mon Dec 29 09:10:15 2014 -0800
+++ b/test/ProblemList.txt Mon Dec 29 13:32:41 2014 -0800
@@ -120,6 +120,10 @@
# jdk_lang
+# 8029891
+java/lang/ClassLoader/deadlock/GetResource.java generic-all
+
+
Thanks,
-Brent
1. https://bugs.openjdk.java.net/browse/JDK-8029891
Re: RFR 8066085: Need a sanity test for rmic -iiop
Hi Felix, Thanks for the updates. I've pushed the changeset. [1] Sorry for the delay; holidays, you know. s'marks [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af229cf4a61a On 12/24/14 7:27 AM, FELIX YANG wrote: Hi Stuart, this is the updated webrev: http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.02/. Please sponsor this changeset for me. Thank you very much, -Felix On 12/24/2014 1:01 PM, Stuart Marks wrote: Hi Felix, Good improvements. I think this is pretty close; I have just a few minor comments. * The test is named IIOPSanityTest but it's in a directory named iiopCompilation. I don't think we need different names. There are many test classes named Foo that reside in a directory named foo; I think we should do the same here. I think keeping the directory name iiopCompilation and renaming the class to IIOPCompilation makes things a bit more descriptive. * Declaring string constants like RMIC_PROGRAM, IIOP_OPTION, and CP_OPTION doesn't really help things, since they're each used exactly once, the string values are unlikely to change, and the symbolic names are just derived from the string vaues. You might as well use string literals directly in each place they're needed. * Printing the command that's about to be executed is a good idea, but the printf statement that prints the command has to be kept in synch with the array containing the command and args. If the command were to change, it'd be easy to forget to update the printf statement or to get it wrong. * I'll also observe that there is an overload of the ProcessBuilder constructor that takes a ListString instead of a String[]. Lists are often easier to work with than arrays, and they have a toString() method that can be used implicitly by print statements. This suggests an alternative for constructing the rmic command line, something like the following: ListString command = Arrays.asList( rmicProgramStr, -iiop, -classpath, testClasses, classname); System.out.println(Running command: + command); After you fix these up I think this will be ready to go in. I can sponsor this changeset for you if you need a sponsor. Thanks, s'marks On 12/19/14 1:56 AM, FELIX YANG wrote: Hi Stuart, please review the updated webrev: http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.01/ Thanks, -Felix On 12/12/2014 4:08 PM, Stuart Marks wrote: On 12/11/14 8:41 PM, FELIX YANG wrote: Hi all, please review the fix to add a sanity checking for rmic -iiop compiling. Bug: https://bugs.openjdk.java.net/browse/JDK-8066085 Webrev: http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.00/ Hi Felix, Thanks for picking up the writing of this test. I have several comments. 1. Usually we try to avoid naming tests after bug IDs. There are such tests in the suite but most tests have more descriptive names. I'd suggest something like iiopSanityTest. 2. There are other rmic tests in jdk/test/sun/rmi/rmic; the test should be moved there (and the path to the TestLibrary updated correspondingly). 3. The test summary should be a bit more explicit. Instead of a sanity test for rmic -iiop compiling I'd suggest something like Compiles a PortableRemoteObject with rmic -iiop and ensures that stub and tie classes are generated. 4. When invoking rmic, you need to make sure to pass the -iiop option. 5. Add check for the _Tie class as well as the _Stub class. Not that it really matters, since we know these classes are in the unnamed package, but the stub and tie class files would end up in directories corresponding to their package names -- if they were in a named package. 6. In doTest(), the try/catch of InterruptedException and IOException and call to TestLibrary.bomb() is mostly unnecessary. (Yes, other RMI tests do this, and I've been removing it where appropriate. In the RMI test library, the bomb() call merely wraps and rethrows the exception, and doesn't really add any value. It's like calling fail() in a test-ng test.) In general an uncaught exception will cause a jtreg test to fail, so you might as well just declare the method with throws Exception and let them propagate. This helps reduce clutter. 7. Similarly I'm not sure it's necessary to check the classname argument for null; the failure should be apparent enough if classname is null. But if you think it's worth it, a concise way to null-check an argument is to use Objects.requireNonNull(). 8. The rmiComplie method is misnamed (or typo'd); I'd suggest something like runRmic. 9. Changing the directory of the rmic subprocess is confusing, because it makes different parts of the program deal with different paths to the same files. In addition, switching directories to the test.classes directory will leave the stub/tie classes there, which could possibly confuse future test runs, since that directory isn't necessarily cleared between test runs. The default working directory of jtreg tests is the scratch directory, which is cleared before each test run.
JDK 9 RFR of 4026465: Provide more byte array constructors for BigInteger
Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-4026465 Patch: http://cr.openjdk.java.net/~bpb/4026465/webrev.00/ Should this issue not look to be worth addressing after all, then I propose that it should be resolved as “Won’t Fix” rather than be left dangling. If on the other hand the change appears reasonable, then a CCC request will be in order. Thanks, Brian
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
I'm looking at this version of the webrev. http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ I just assigned 8047769 to you. My username is wetmore, Alan is alanb. On 12/24/2014 3:37 AM, Peter Levart wrote: Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I have started a JPRT job for you, I'll run it through core target which will give us: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. Anything else? I'm off tomorrow, I should have full results Wed. Here are the preliminary results for the jobs that have finished. jdk.testlibrary.Asserts is causing compilation errors, additional comments below: /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:33: error: package jdk.testlibrary does not exist import static jdk.testlibrary.Asserts.*; ^ /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:52: error: cannot find symbol assertEquals(bytes.length, nread, short read); ^ symbol: method assertEquals(int,int,String) location: class FileInputStreamPoolTest /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:53: error: cannot find symbol assertTrue(Arrays.equals(readBytes, bytes), ^ symbol: method assertTrue(boolean,String) location: class FileInputStreamPoolTest 3 errors TEST RESULT: Failed. Compilation failed: Compilation failed I'm also getting failures in the following test. I unfortunately have to leave now, so don't have time to look at this. But it did mention seed so I'm mentioning it here. TEST: java/lang/invoke/LFCaching/LFGarbageCollectedTest.java ACTION: main -- Failed. Execution failed: `main' threw exception: java.lang.Error: 36 of 39 test cases FAILED! Rerun the test with the same -Dseed= option as in the log file! REASON: User specified action: run main/othervm LFGarbageCollectedTest TIME: 213.406 seconds messages: command: main LFGarbageCollectedTest reason: User specified action: run main/othervm LFGarbageCollectedTest elapsed time (seconds): 213.406 STDOUT: -Dseed=6102311124531075592 -DtestLimit=2000 Number of iterations according to -DtestLimit is 153 (1989 cases) Code cache size is 251658240 bytes Non-profiled code cache size is 123058253 bytes Number of iterations limited by code cache size is 84 (1092 cases) Number of iterations limited by non-profiled code cache size is 41 (533 cases) Number of iterations is set to 41 (533 cases) Not enough time to continue execution. Interrupted. STDERR: Iteration 0: Tested LF caching feature with MethodHandles.foldArguments method. java.lang.AssertionError: Error: Lambda form is not garbage collected at LFGarbageCollectedTest.doTest(LFGarbageCollectedTest.java:81) at LambdaFormTestCase$TestRun.doIteration(LambdaFormTestCase.java:139) at LambdaFormTestCase$$Lambda$2/5042013.call(Unknown Source) at jdk.testlibrary.TimeLimitedRunner.call(TimeLimitedRunner.java:71) at LambdaFormTestCase.runTests(LambdaFormTestCase.java:201) at LFGarbageCollectedTest.main(LFGarbageCollectedTest.java:105) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) In a couple places, there are a few lines 80 chars. (It's a pet peeve of mine, this makes side-by-side reviews difficult without a wide monitor. I realize not everyone shares this view, but they're probably not working on a laptop screen regularly.) I have wrapped the lines to contain them inside the 80 column margin. I and my scrubber/slider thank you. :) Two minor nits? SeedGenerator.java: Lines 507/518 Do you need to close the InputStream when last holder is GC'd, or do we just let the FileInputStream's finalizer take care of that? WeakReferenceUncloseableInputStream is enqueued when it is cleared, so at that time we have no access to the referent (UncloseableInputStream) any more. We could, in addition, strongly reference the underlying FileInputStream in the WeakReference subclass itself, but that would just prolong the life of FileInputStream (possibly forever if no further calls to FileInputStreamPool are made that expunge the references from the queue). So yes, we rely on FileInputStream's finalizer, but I don't see any other way that would be better than that. Makes sense, thanks. NativePRNG and URLSeedGenerator don't have a means of closing underlying resources either, so this is not making things any worse. Yes. Both of these current calls are contained within a AccessContrller.doPriviledged, the checkRead() seems redundant. That's right, but from
