RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors

Thu Jan 1 20:22:20 UTC 2015

Hi Brad,

Here's next webrev which tries to cover all your comments:

http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.04/

Answers inline...

On 12/30/2014 02:48 AM, Bradford Wetmore wrote:
> 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.

I'll note you both as reviewers in the changeset.

>
> 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'm worried about the failure of the test you observed while running 
from NetBeans. Perhaps a 0.5s wait is sometimes not enough for 
ReferenceHandler thread to process (enqueue) a WeakReference. Since 
there is already a facility in place to help ReferenceHandler thread 
instead of wait for it, I used it in new version of the test.

> 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 changed the test to be self-contained now so one can run it without 
testlib in classpath.

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

Done that too.

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

This should work now.

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

Done.

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

Fixed.

>
> Maybe issue multiple reads to exercise in1 and in2?  e.g. 2 bytes of 
> in1, 4 bytes of in2, then 2 bytes of in1?

The 1st assert makes sure in1 == in2, so there's no point in invoking 
the same instance via two aliases.

>
> IIRC, when I ran this under NetBeans last week, the second testCaching 
> didn't clear the WeakReference.

This should not happen any more now that the test is helping to enqueue 
the WeakReferences instead of waiting for ReferenceHandler to enqueue 
them. The test can now fail only if System.gc() does not trigger 
WeakReference processing in the VM. Can you give it a try on your 
NetBeans environment?

>
> Thanks,
>
> Brad

Regards, Peter

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150101/76072d2f/attachment.htm>