Yes, that's why I said it's not implemented correctly, but it has worked
fine until this bug appears.
--Max
On 8/31/2016 9:44, Bernd Eckenfels wrote:
Hello,
readBytes read all bytes of a specified length. If the length is not
known it is not usefull to use. Besides it is also not usefull to only
read available() bytes, as the nmber of bytes read might be up to all
kinds of factors (including io request and tcp segment boundaries).
It is not a valid abstraction to model records basd on read sizes. I
think using available() is almost always a bug. (But I am not sure what
the specific datastructures involved are).
Gruss
Bernd
Am Wed, 31 Aug 2016 09:18:23 +0800
schrieb Weijun Wang <weijun.w...@oracle.com>:
On 8/31/2016 0:46, Svetlana Nikandrova wrote:
Hi Max,
thank you for your replay. Please see inline.
On 30.08.2016 5:43, Weijun Wang wrote:
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?
readAllBytes() blocks until all remaining data has been read and
end of stream is detected.
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?
The KeyStore::load method makes no assumption on the InputStream
parameter. More important, DerValue is used much wider than pkcs12
keystore.
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.
Yes, I think it is possible to read data by portions, but
unfortunately straight forward approach like
"while(is.available() !=0)" 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.
Well, this is unfortunate.
I also cannot think of a simple fix now. Maybe after see that "not
all indef len BER resolved" error you can try read more.
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.
It asks for some investigation. Should I create a separate issue?
You can, but please set the target version to major-tbd unless you
want to work on it now.
Thanks
Max
Thank you,
Svetlana
Thanks
Max
On 8/30/2016 2:52, Artem Smotrakov wrote:
Hi Svetlana,
It looks fine, but I am not an official reviewer.
"keystorePath" in readTest() can be a static field.
I also meant that one test with SequenceInputStream seems to be
enough, so you could just add a new test case to
ReadP12Test.java. But it's fine.
I am not sure how DerIndefLenConverter works, but it looks a
little strange to me that it needs to extend an array before
passing it to DerIndefLenConverter. I see that convert() method
also uses arraycopy() method. But it seems to be out of scope
here.
Artem
On 08/29/2016 11:23 AM, Svetlana Nikandrova wrote:
Hi Artem,
thank you for your replay. I've updated copyright and made
separate test for this bug.
As for Arrays.copyOfRange() unfortunately it won't simplify code
in my case. I need to extend an array, not to get a sub-array of
existing one.
http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/
<http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/>
Thanks,
Svetlana
On 26.08.2016 23:48, Artem Smotrakov wrote:
Hi Svetlana,
DerValue class may be implicitly used in different areas (x509,
SSL/TLS, keystores, maybe krb5, etc). Please make sure that
tests from jdk_security pass.
I'll leave the main review to someone who is more knowledgeable
in this area, here are a couple of comments:
- Please update copyright year
- You may want to replace new byte[] + System.arraycopy() by
Arrays.copyOfRange()
- It may be better to add a separate test case in
ReadP12Test.java for SequenceInputStream instead of loading a
keystore twice in each call to readTest(). One test with
SequenceInputStream seems to be enough, and it would make the
logic of readTest() clearer.
Artem
On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:
Hello,
please review this fix. It's not possible to read PKCS12
keystore with big undefined length DER value in it from
SequenceInputStream. Root cause of the problem is that
sun.security.util.DerValue relays on InputStream.available()
to get a complete 'indefinite.length' section length and then
read it, but for SequenceInputStream this method returns
number of available bytes only for current input stream, not
the whole sequence. Fixed to read all available data.
JBS:
https://bugs.openjdk.java.net/browse/JDK-8157404
Webrev:
http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/
<http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/>
Thanks,
Svetlana
