Handle case where UDP packet contains multiple DTLS records.

Handle case where UDP packet contains multiple DTLS records.

Our DTLS implementation doesn't do this, but other implementations may; see https://tools.ietf.org/html/rfc6347#section-4.1.1.

BUG=chromium:537189
Review-Url: https://codereview.webrtc.org/2970883005
Cr-Commit-Position: refs/heads/master@{#18934}
Committed: https://chromium.googlesource.com/external/webrtc/+/7d0a77eef431748f26cacee140bef3b36cc9bc22

[joachim, 2017-07-05 22:43:27]

jbauch@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Taylor Brandstetter, 2017-07-06 21:47:52]

Description was changed from

==========
Handle case where UDP packet contains multiple DTLS records.

BUG=537189
==========

to

==========
Handle case where UDP packet contains multiple DTLS records.

BUG=chromium:537189
==========

[Taylor Brandstetter, 2017-07-06 22:06:11]

lgtm; I just think some more comments would be good

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:541: do {
Could use a comment explaining why this is done in a loop.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:673: // Connect with DTLS,
combine multiple DTLS records into one packet.
May be nice to have some context here; "Our DTLS implementation doesn't do this,
but other implementations may; see
https://tools.ietf.org/html/rfc6347#section-4.1.1."

Also, do you know what other implementations are known for doing this? Are they
not using OpenSSL/BoringSSL? Or are they using some option we don't know about?

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:678:
transport->set_combine(true);
Would be nice to have a comment saying something like: "Our DTLS implementation
always sends one record per packet, so to simulate an endpoint that sends
multiple records per packet, we configure the fake ICE transport to combine
every two consecutive packets into a single packet."

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
File webrtc/p2p/base/fakeicetransport.h (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:164: // If combine is enabled, two incoming
packets will be combined into one
This comment is a little misleading, since all the packets here are outgoing.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:166: void set_combine(bool combine) {
combine_ = combine; }
nit: Making this name more descriptive, like "combine_outgoing_packets",
wouldn't hurt.

[joachim, 2017-07-07 16:13:00]

All comments addressed.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:541: do {
On 2017/07/06 22:06:11, Taylor Brandstetter wrote:
> Could use a comment explaining why this is done in a loop.

Done.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:673: // Connect with DTLS,
combine multiple DTLS records into one packet.
On 2017/07/06 22:06:11, Taylor Brandstetter wrote:
> May be nice to have some context here; "Our DTLS implementation doesn't do
this,
> but other implementations may; see
> https://tools.ietf.org/html/rfc6347#section-4.1.1.%22

Done

> Also, do you know what other implementations are known for doing this? Are
they
> not using OpenSSL/BoringSSL? Or are they using some option we don't know
about?

According to https://crbug.com/537189 this seems to be an interop problem with
ORTCLib (I mention this in the comment now). On their website
(http://ortclib.org/) they write about using OpenSSL for security, but I don't
know any details about their actual DTLS implementation.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:678:
transport->set_combine(true);
On 2017/07/06 22:06:11, Taylor Brandstetter wrote:
> Would be nice to have a comment saying something like: "Our DTLS
implementation
> always sends one record per packet, so to simulate an endpoint that sends
> multiple records per packet, we configure the fake ICE transport to combine
> every two consecutive packets into a single packet."

Done.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
File webrtc/p2p/base/fakeicetransport.h (right):

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:164: // If combine is enabled, two incoming
packets will be combined into one
On 2017/07/06 22:06:11, Taylor Brandstetter wrote:
> This comment is a little misleading, since all the packets here are outgoing.

Changed.

https://codereview.webrtc.org/2970883005/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:166: void set_combine(bool combine) {
combine_ = combine; }
On 2017/07/06 22:06:11, Taylor Brandstetter wrote:
> nit: Making this name more descriptive, like "combine_outgoing_packets",
> wouldn't hurt.

Done.

[joachim, 2017-07-07 16:13:32]

Description was changed from

==========
Handle case where UDP packet contains multiple DTLS records.

BUG=chromium:537189
==========

to

==========
Handle case where UDP packet contains multiple DTLS records.

Our DTLS implementation doesn't do this, but other implementations may; see
https://tools.ietf.org/html/rfc6347#section-4.1.1.

BUG=chromium:537189
==========

[Taylor Brandstetter, 2017-07-07 20:01:07]

lgtm, and thanks for fixing this!

[joachim, 2017-07-07 20:19:44]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2017-07-07 20:19:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-07 20:44:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499458784101410,
"parent_rev": "7480da4118b72b3a64f007cac6556dc64d5682ca", "commit_rev":
"7d0a77eef431748f26cacee140bef3b36cc9bc22"}

[commit-bot: I haz the power, 2017-07-07 20:44:13]

Description was changed from

==========
Handle case where UDP packet contains multiple DTLS records.

Our DTLS implementation doesn't do this, but other implementations may; see
https://tools.ietf.org/html/rfc6347#section-4.1.1.

BUG=chromium:537189
==========

to

==========
Handle case where UDP packet contains multiple DTLS records.

Our DTLS implementation doesn't do this, but other implementations may; see
https://tools.ietf.org/html/rfc6347#section-4.1.1.

BUG=chromium:537189

Review-Url: https://codereview.webrtc.org/2970883005
Cr-Commit-Position: refs/heads/master@{#18934}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7d0a77eef431748f26cacee14...
==========

[commit-bot: I haz the power, 2017-07-07 20:44:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/7d0a77eef431748f26cacee14...