<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Max,<br>
<br>
thank you for your replay. Please see inline. <br>
<br>
<div class="moz-cite-prefix">On 30.08.2016 5:43, Weijun Wang wrote:<br>
</div>
<blockquote
cite="mid:d911ed7f-e812-6586-4820-efd60092731a@oracle.com"
type="cite">readAllBytes() wants to real *all* data. If this is
during a long time communication, and the peer sends a BER using
indefinite length, and then wait for the next round of dialog
without closing the stream, will readAllBytes() hang?
<br>
</blockquote>
<br>
readAllBytes() blocks until all remaining data has been read and end
of stream is detected. <br>
So if I understood your scenario correctly, yes, if EOF is not
present it will hang. AFAIK when KeyStore.load completes
successfully it consumes the input stream until EOF, so isn't it ok
to expect it?
<br>
<br>
<blockquote
cite="mid:d911ed7f-e812-6586-4820-efd60092731a@oracle.com"
type="cite">Is it possible to create a new method
readAllAvailables() here? It kept read data until available() is
zero and return the concatenated byte array. I think this method
does not block and is able to read everything available from a
SequenceInputStream.
<br>
</blockquote>
<br>
Yes, I think it is possible to read data by portions, but
unfortunately straight forward approach like "while<span
style="color:#000080;font-weight:bold;"> </span>(is.available()
!=0<span style="color:#0000ff;"></span>)" won't work. As soon as we
reach the end of the first stream in SequenceInputStream sequence
is.available() will return 0 until we switch to the next stream by
calling read(). I'm afraid it will look a little bit confusing. <br>
<br>
<blockquote
cite="mid:d911ed7f-e812-6586-4820-efd60092731a@oracle.com"
type="cite">In fact, IMO the DerValue is not implemented
correctly. It should have been able to read the input data chunk
by chunk until a BER is fully consumed, leaving data after the BER
untouched, and block if the BER is not complete. To achieve this,
it should read the header, read zero or more sub-data, read the
EOC, recursively if necessary.
<br>
</blockquote>
<br>
It asks for some investigation. Should I create a separate issue?<br>
<br>
Thank you,<br>
Svetlana <br>
<br>
<blockquote
cite="mid:d911ed7f-e812-6586-4820-efd60092731a@oracle.com"
type="cite">
<br>
Thanks
<br>
Max
<br>
<br>
On 8/30/2016 2:52, Artem Smotrakov wrote:
<br>
<blockquote type="cite">Hi Svetlana,
<br>
<br>
It looks fine, but I am not an official reviewer.
<br>
<br>
"keystorePath" in readTest() can be a static field.
<br>
<br>
I also meant that one test with SequenceInputStream seems to be
enough,
<br>
so you could just add a new test case to ReadP12Test.java. But
it's fine.
<br>
<br>
I am not sure how DerIndefLenConverter works, but it looks a
little
<br>
strange to me that it needs to extend an array before passing it
to
<br>
DerIndefLenConverter. I see that convert() method also uses
arraycopy()
<br>
method. But it seems to be out of scope here.
<br>
<br>
Artem
<br>
<br>
<br>
On 08/29/2016 11:23 AM, Svetlana Nikandrova wrote:
<br>
<blockquote type="cite">Hi Artem,
<br>
<br>
thank you for your replay. I've updated copyright and made
separate
<br>
test for this bug.
<br>
As for Arrays.copyOfRange() unfortunately it won't simplify
code in my
<br>
case. I need to extend an array, not to get a sub-array of
existing one.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/">http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/"><http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/></a>
<br>
<br>
Thanks,
<br>
Svetlana
<br>
<br>
On 26.08.2016 23:48, Artem Smotrakov wrote:
<br>
<blockquote type="cite">
<br>
Hi Svetlana,
<br>
<br>
DerValue class may be implicitly used in different areas
(x509,
<br>
SSL/TLS, keystores, maybe krb5, etc). Please make sure that
tests
<br>
from jdk_security pass.
<br>
<br>
I'll leave the main review to someone who is more
knowledgeable in
<br>
this area, here are a couple of comments:
<br>
- Please update copyright year
<br>
- You may want to replace new byte[] + System.arraycopy() by
<br>
Arrays.copyOfRange()
<br>
- It may be better to add a separate test case in
ReadP12Test.java
<br>
for SequenceInputStream instead of loading a keystore twice
in each
<br>
call to readTest(). One test with SequenceInputStream seems
to be
<br>
enough, and it would make the logic of readTest() clearer.
<br>
<br>
Artem
<br>
<br>
On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:
<br>
<blockquote type="cite">Hello,
<br>
<br>
please review this fix. It's not possible to read PKCS12
keystore
<br>
with big undefined length DER value in it from
SequenceInputStream.
<br>
Root cause of the problem is that
sun.security.util.DerValue relays
<br>
on InputStream.available() to get a complete
'indefinite.length'
<br>
section length and then read it, but for
SequenceInputStream this
<br>
method returns number of available bytes only for current
input
<br>
stream, not the whole sequence. Fixed to read all
available data.
<br>
<br>
JBS:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8157404">https://bugs.openjdk.java.net/browse/JDK-8157404</a>
<br>
Webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/">http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/"><http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/></a>
<br>
<br>
Thanks,
<br>
Svetlana
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>