RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
Bradford Wetmore
bradford.wetmore at oracle.com
Tue Dec 30 01:48:37 UTC 2014
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?
>
> WeakReference<UncloseableInputStream> 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 encapsulation, uniformity, security and future
> maintainability standpoint, I would keep this logic in. It doesn't hurt.
> Another possibility is to move doPriviliged call to FileInputStreamPool
> itself and declare it's API exposing security capability (of reading any
> file).
I see this was addressed later via Alan's review.
>> In your test case, if assertions are not enabled, the tests at
>> 46/50/51 are noops, e.g. when run by hand. Generally should directly
>> throw Exceptions.
>
> I modified the test to use jdk.testlibrary.Asserts class. Is this ok
> from "run by hand" perspective or should the test be self-contained?
I've not used this Asserts library yet. Is this part of TestNG, or
something new in jtreg or jprt? If Core-libs is ok with this style of
doing it, I'm ok. I'm kind of old-school and tests should be mostly
self-contained and can be tested via:
% javac Clazz.java
% java Clazz
without extra classpaths needed. I understand this model doesn't work
with @library and such, so I'm not strongly tied to it. I can be taught
new tricks.
>> Core-libs folks?
>
> The documentation doesn't mention threads anywhere in InputStream or
> FileInputStream except in this piece of javadoc for available() method:
...snip...
Ok.
A few minor nits below:
FileInputStreamPool.java
========================
* This method opens an underlying {@link java.io.FileInputStream} for
->
* This method opens an underlying {@link java.io.FileInputStream} for a
* among multiple readers of same {@code file} and ignores
->
* among multiple readers of the same {@code file} and ignores
FileInputStreamPoolTest.java
============================
Generally JTREG labels are immediately following the copyright and
before the imports.
While what you have is allowed by the JTREG syntax, @test is usually by
itself, or followed by old SCCS or filename info.
@summary is usually the bug description. Suggest:
@summary SecureRandom should be more frugal with file descriptors
48: This is still using assert.
Maybe issue multiple reads to exercise in1 and in2? e.g. 2 bytes of
in1, 4 bytes of in2, then 2 bytes of in1?
IIRC, when I ran this under NetBeans last week, the second testCaching
didn't clear the WeakReference.
Thanks,
Brad
More information about the security-dev
mailing list