[9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout
Xuelei Fan
xuelei.fan at oracle.com
Tue Aug 2 04:35:29 UTC 2016
DTLSOutputRecord
----------------
182 int len = destination.remaining();
235 int len = destination.remaining();
This variable is only use by debug block. I may try to avoid to define
it out side of the debug block. I know you want to dump the length in
the "record" debug option. But I think the epoch/sequence# belong to
the "packet" debug option. The "record" option does not include the
header part, so may be, it is not suitable to dump the epoch/seq# here.
I may suggest to remove the update as the "packet" option can help if
users want to know the epoch/sequence numbers. Please see the comment
for DTLSOverDatagram.java, too.
466-470
This is a nice update to get the user friendly handshake type
description. There are still a lot similar debug log that need to get
similar update. Would you mind to file a new RFE, and make the update
together with other places (both TLS and DTLS) all together?
DTLSInputRecord
---------------
159 long onlySeqNum =
Authenticator.getDTLSSequenceNumber(recordEnS);
Using getDTLSSequenceNumber(long) may be more effective.
161-165, if you consider to remove the debug log update in
DTLSOutputRecord, you may also want to remove the debug log update here
for similar reasons.
HandshakeMessage
----------------
See comment for DTLSOutputRecord. I may prefer to file a new RFE, and
make the update all together.
Authenticator
-------------
May not need the update any more if you consider my suggestions above.
Otherwise, I think may be only
getDTLSSequenceNumber(long recordEnS)
is actually used. Other two new methods can be removed.
InvalidRecords
--------------
I like this find!
DTLSOverDatagram
----------------
50 System.setProperty("javax.net.debug", "ssl,record");
I may suggest to use "ssl,record,packet" for debugging if you won't dump
the epoch/seq# in "record" debug option. If the test passed, I may
prefer to turn the debug log off.
doServerSide()
I did not get too much about the update:
1. Is it necessary to combine handshake and reading client application
data together?
I think the receiveAppData can be reused.
2. Is it necessary to re-send application data?
I think if application data is not allowed to drop, may not need to
the re-send code.
Same for the doClientSide() implementation.
disablePacketDrop() is a static method, you may not want to call it
exception the initialization stage. The following line need to be very
carefully used:
162 CustomDatagramSocketImplFactory.disablePacketDrop();
334 CustomDatagramSocketImplFactory.disablePacketDrop();
324-375, may be not necessary, or can be improved to use
receiveAppData() method instead. I think the wrap() and unwrap() impl
should take care of the retransmission of handshaking messages, but not
the application. Resending the previous packet does not comply to DTLS
retransmission specification. If you run into problems, would you mind
show me the debug log?
>> - Updated client and server to use simple threads instead of thread
>> pools. This way, it's possible to specify meaningful names for
>> threads
A more light update may be use Thread.setName(String name) in the
ServerCallable.call(). I did not test. If you like, you can have a try.
CustomDatagramSocketImpl.addPacket()
I may suggest to check application data here.
927 private void addPacket(byte[] data) {
928 // check if a packet should be dropped
- 929 if (CustomDatagramSocketImplFactory.losePacket()) {
+ if (ustomDatagramSocketImplFactory.losePacket() &&
+ (data[0] != (byte)0x17)) {
930 return;
931 }
"data[0] != (byte)0x17" means that it is not application data. As would
simplify the code a lot. See comments above about the application data
re-sending issue.
PacketLossXXX.java
------------------
It should not be the common case to split test cases separated with the
test code. I would suggest merge them into the PacketLoss.java. If you
are worrying about the debug log length, please turn off the debug log.
Actually, we should turn off the debug log unless we need more detailed
debug log for problematic test. Or alternatively, we can ask the JTREG
to consider the behavior more about the debug length restrictions.
Thanks,
Xuelei
On 8/2/2016 9:25 AM, Artem Smotrakov wrote:
> Here is an updated webrev which includes a fix for 8161086 (thanks
> Xuelei for help):
> - No updates to the problem list
> - CustomDatagramSocketImplFactory drops 5% of packets by default
> - Updated DTLSInputRecord and DTLSOutputRecord to print sequence numbers
> if -Djavax.net.debug=ssl option is specified
> - Updated InvalidRecords.java tests not to drop packets by default
> because handshaking unexpectedly succeeds if an invalid packet was dropped
>
> http://cr.openjdk.java.net/~asmotrak/8159416/webrev.03/
>
> Please take a look.
>
> Artem
>
> On 07/28/2016 04:36 PM, Artem Smotrakov wrote:
>> Hi Xuelei,
>>
>> I updated CustomDatagramSocketImpl class to be able to drop some
>> packets randomly or by specified numbers.
>>
>> While updating the test, I found
>> https://bugs.openjdk.java.net/browse/JDK-8161086 which I think is the
>> root cause of these intermittent failures.
>>
>> I added a couple of PacketLoss* tests to reproduce and verify 8161086.
>> These tests drop different packets while handshaking. They use
>> different cipher suites and modes, so that they take a while. I didn't
>> notice they fail with timeout, but I suspect that may happen on slower
>> machines. That's why I increased timeout value for them. I added the
>> tests to the problem list since 8161086 is not fixed yet.
>>
>> There are a couple of other updates to DTLSOverDatagram.java:
>> - Server tries to receive first application data when it finishes
>> handshaking. Server re-sends final handshaking messages if timeout
>> reached while waiting for application data from client.
>> - Updated client and server to use simple threads instead of thread
>> pools. This way, it's possible to specify meaningful names for threads
>> which makes logs more readable if "-Djavax.net.debug=ssl" specified
>> - Added more logging
>> - Made some fields final
>>
>> At the moment, CustomDatagramSocketImplFactory doesn't drop packets by
>> default because it may cause intermittent failures of tests which are
>> based on DTLSOverDatagram class:
>>
>> ...
>> static class CustomDatagramSocketImplFactory
>> implements DatagramSocketImplFactory {
>> ...
>> // if true, it's going to drop some packets
>> private static boolean dropPackets = false;
>> ...
>>
>> When 8161086 is fixed, it may be set to true by default. Currently the
>> packet loss rate is 5% which I think is much more than if we use UDP
>> sockets on localhost, so that we test DTLS impl in worse conditions.
>>
>> Could you please take a look at updated webrev?
>>
>> http://cr.openjdk.java.net/~asmotrak/8159416/webrev.02/
>>
>> Artem
>>
>> On 06/27/2016 06:25 PM, Xuelei Fan wrote:
>>> On 6/28/2016 9:12 AM, Artem Smotrakov wrote:
>>>> Hello,
>>>>
>>>> Please review this patch for javax/net/ssl/DTLS tests.
>>>>
>>>> A couple of DTLS tests fail intermittently on Mac with timeout or "Too
>>>> many handshake loops ..." error. The tests use UDP to transfer DTLS
>>>> records. It happens sometimes that server cannot receive UDP packets
>>>> with DatagramSocket.receive() method even if client tries to re-send
>>>> them multiple times. Please see logs in the bug.
>>>>
>>>> At the moment, it is not clear why UDP packets can't be delivered to
>>>> server. If someone has seen something similar before, or has some ideas
>>>> what might be the root cause, please let me know.
>>>>
>>> UDP is not reliable. It could happen that the packets get lost.
>>>
>>>> I think that the tests and DTLS implementation are fine here. Since the
>>>> tests are for DTLS, we can workaround this issue by avoiding real UDP
>>>> connections. To avoid changing test logic much, we can use a custom
>>>> DatagramSocketImpl and DatagramSocketImplFactory implementations to
>>>> replace system UDP implementation.
>>>>
>>>> The patch below updates the tests with the following:
>>>> - added custom DatagramSocketImpl and DatagramSocketImplFactory
>>>> implementations to avoid real UDP connections
>>> Tests for real connections in practice are needed. If we update this
>>> test this way, we need to add other tests to test real application
>>> usage. I don't think this is the right direction to avoid real UDP
>>> connections.
>>>
>>>> - added more test output, so that we can have more info it the tests
>>>> fail again
>>>>
>>> I agree with this point.
>>>
>>> Xuelei
>>>
>>>> I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac,
>>>> and I didn't see it failed. I also have run javax/net/ssl/DTLS tests on
>>>> all supported generic platforms, and they worked fine.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
>>>> http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/
>>>>
>>>> Artem
>>
>
More information about the security-dev
mailing list