Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by Taylor Brandstetter Modified:
4 years, 7 months ago Reviewers:
pthatcher1, honghaiz3, juberti2 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCache a ClientHello received before the DTLS handshake has started.
In some cases, the DTLS ClientHello may arrive before the server's
transport is writable (before it receives a STUN ping response), or
even before it receives a remote fingerprint. If this packet is
discarded, it may take a second for a it to be sent again.
So, this CL caches it instead of dropping it, and feeds it into
the SSL library once the handshake has been started.
BUG=webrtc:5789
Committed: https://crrev.com/e84cd2eacad78c67b8c31912abc40c14314d4175
Cr-Commit-Position: refs/heads/master@{#12634}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Adding tests for receiving a ClientHello before the channel is writable, or before getting a remote… #Patch Set 3 : Fixing typo. #
Total comments: 2
Patch Set 4 : Fixing typos in comments. #
Created: 4 years, 7 months ago
Messages
Total messages: 26 (9 generated)
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
PTAL. By the way, I don't have any unit tests because it would be hard to write one that's not flaky, since this is so time-sensitive. But I have a CL that adds a simulated clock, which would solve that problem: https://codereview.webrtc.org/1895933003/
On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > PTAL. > > By the way, I don't have any unit tests because it would be hard to write one > that's not flaky, since this is so time-sensitive. > > But I have a CL that adds a simulated clock, which would solve that problem: > https://codereview.webrtc.org/1895933003/ This will work but I wonder if there is a better way to fix this and the other issue Justin mentioned that the DTLS packets may be silently dropped if the underlying transport channel is not writable. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... The real problem here is that the DTLS channel may be readable but not writable. What we are doing here is trying to cache the packet received in this case. Can we instead cache the packets that will be sent out when the channel is not writable? By doing that, we may fix both issues. If that is too complicated, I am OK with this one.
juberti@chromium.org changed reviewers: + juberti@chromium.org
Needs unit test https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:479: if (IsDtlsPacket(data, size)) { We should probably also verify this is a CLIENT HELLO, at least for logging. https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:592: LOG_J(LS_WARNING, this) << "Discarding cached DTLS packet because " Log text could be clearer to indicate this is a real malfunction by the remote side as opposed to just a race condition or some such. https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.h:242: rtc::Buffer cached_dtls_packet_; Might want to call this cached_client_hello_, if we decide to only do this for client hellos
On 2016/04/25 19:56:32, honghaiz3 wrote: > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > PTAL. > > > > By the way, I don't have any unit tests because it would be hard to write one > > that's not flaky, since this is so time-sensitive. > > > > But I have a CL that adds a simulated clock, which would solve that problem: > > https://codereview.webrtc.org/1895933003/ > > This will work but I wonder if there is a better way to fix this and the other > issue Justin mentioned > that the DTLS packets may be silently dropped if the underlying transport > channel is not writable. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > The real problem here is that the DTLS channel may be readable but not writable. > > What we are doing here is trying to cache the packet received in this case. > Can we instead cache the packets that will be sent out when the channel is not > writable? > By doing that, we may fix both issues. > If that is too complicated, I am OK with this one. I assumed it would be unlikely for a channel to become unwritable before the DTLS handshake finished. Is that a bad assumption? Or, are there DTLS packets sent after the handshake finishes? Also, even if we cached outgoing packets, we would still want to cache the CLIENT HELLO in case it's received before the SDP answer which contains the remote fingerprint.
On 2016/04/26 01:32:14, Taylor Brandstetter wrote: > On 2016/04/25 19:56:32, honghaiz3 wrote: > > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > > PTAL. > > > > > > By the way, I don't have any unit tests because it would be hard to write > one > > > that's not flaky, since this is so time-sensitive. > > > > > > But I have a CL that adds a simulated clock, which would solve that problem: > > > https://codereview.webrtc.org/1895933003/ > > > > This will work but I wonder if there is a better way to fix this and the other > > issue Justin mentioned > > that the DTLS packets may be silently dropped if the underlying transport > > channel is not writable. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > The real problem here is that the DTLS channel may be readable but not > writable. > > > > What we are doing here is trying to cache the packet received in this case. > > Can we instead cache the packets that will be sent out when the channel is not > > writable? > > By doing that, we may fix both issues. > > If that is too complicated, I am OK with this one. > > I assumed it would be unlikely for a channel to become unwritable before the > DTLS handshake finished. Is that a bad assumption? Or, are there DTLS packets > sent after the handshake finishes? > > Also, even if we cached outgoing packets, we would still want to cache the > CLIENT HELLO in case it's received before the SDP answer which contains the > remote fingerprint. I don't think you need to worry about that. Caching the client hello solves the readable but not writable case, since we don't try to start DTLS until writable.
On 2016/04/26 05:47:21, juberti2 wrote: > On 2016/04/26 01:32:14, Taylor Brandstetter wrote: > > On 2016/04/25 19:56:32, honghaiz3 wrote: > > > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > > > PTAL. > > > > > > > > By the way, I don't have any unit tests because it would be hard to write > > one > > > > that's not flaky, since this is so time-sensitive. > > > > > > > > But I have a CL that adds a simulated clock, which would solve that > problem: > > > > https://codereview.webrtc.org/1895933003/ > > > > > > This will work but I wonder if there is a better way to fix this and the > other > > > issue Justin mentioned > > > that the DTLS packets may be silently dropped if the underlying transport > > > channel is not writable. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > The real problem here is that the DTLS channel may be readable but not > > writable. > > > > > > What we are doing here is trying to cache the packet received in this case. > > > Can we instead cache the packets that will be sent out when the channel is > not > > > writable? > > > By doing that, we may fix both issues. > > > If that is too complicated, I am OK with this one. > > > > I assumed it would be unlikely for a channel to become unwritable before the > > DTLS handshake finished. Is that a bad assumption? Or, are there DTLS packets > > sent after the handshake finishes? > > > > Also, even if we cached outgoing packets, we would still want to cache the > > CLIENT HELLO in case it's received before the SDP answer which contains the > > remote fingerprint. > > I don't think you need to worry about that. Caching the client hello solves the > readable but not writable case, since we don't try to start DTLS until writable. For the unit test, you should be able to set the FakeTransportChannel to writable only on the callee side, and then set the caller to writable after the client hello packet is sent. I don't think you need the fake clock.
On 2016/04/26 17:15:59, juberti2 wrote: > On 2016/04/26 05:47:21, juberti2 wrote: > > On 2016/04/26 01:32:14, Taylor Brandstetter wrote: > > > On 2016/04/25 19:56:32, honghaiz3 wrote: > > > > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > > > > PTAL. > > > > > > > > > > By the way, I don't have any unit tests because it would be hard to > write > > > one > > > > > that's not flaky, since this is so time-sensitive. > > > > > > > > > > But I have a CL that adds a simulated clock, which would solve that > > problem: > > > > > https://codereview.webrtc.org/1895933003/ > > > > > > > > This will work but I wonder if there is a better way to fix this and the > > other > > > > issue Justin mentioned > > > > that the DTLS packets may be silently dropped if the underlying transport > > > > channel is not writable. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > The real problem here is that the DTLS channel may be readable but not > > > writable. > > > > > > > > What we are doing here is trying to cache the packet received in this > case. > > > > Can we instead cache the packets that will be sent out when the channel is > > not > > > > writable? > > > > By doing that, we may fix both issues. > > > > If that is too complicated, I am OK with this one. > > > > > > I assumed it would be unlikely for a channel to become unwritable before the > > > DTLS handshake finished. Is that a bad assumption? Or, are there DTLS > packets > > > sent after the handshake finishes? > > > > > > Also, even if we cached outgoing packets, we would still want to cache the > > > CLIENT HELLO in case it's received before the SDP answer which contains the > > > remote fingerprint. > > > > I don't think you need to worry about that. Caching the client hello solves > the > > readable but not writable case, since we don't try to start DTLS until > writable. > > For the unit test, you should be able to set the FakeTransportChannel to > writable only on the callee side, and then set the caller to writable after the > client hello packet is sent. I don't think you need the fake clock. The reason a unit test would be difficult is that it would likely involve "EXPECT_TRUE_WAIT(dtls_done, kSomeTimeout)". But on a slow bot (MSan/ASan/etc.) the timeout may expire just because the bot is slow, even if things are working as expected.
On 2016/04/26 18:36:16, Taylor Brandstetter wrote: > On 2016/04/26 17:15:59, juberti2 wrote: > > On 2016/04/26 05:47:21, juberti2 wrote: > > > On 2016/04/26 01:32:14, Taylor Brandstetter wrote: > > > > On 2016/04/25 19:56:32, honghaiz3 wrote: > > > > > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > > > > > PTAL. > > > > > > > > > > > > By the way, I don't have any unit tests because it would be hard to > > write > > > > one > > > > > > that's not flaky, since this is so time-sensitive. > > > > > > > > > > > > But I have a CL that adds a simulated clock, which would solve that > > > problem: > > > > > > https://codereview.webrtc.org/1895933003/ > > > > > > > > > > This will work but I wonder if there is a better way to fix this and the > > > other > > > > > issue Justin mentioned > > > > > that the DTLS packets may be silently dropped if the underlying > transport > > > > > channel is not writable. > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > The real problem here is that the DTLS channel may be readable but not > > > > writable. > > > > > > > > > > What we are doing here is trying to cache the packet received in this > > case. > > > > > Can we instead cache the packets that will be sent out when the channel > is > > > not > > > > > writable? > > > > > By doing that, we may fix both issues. > > > > > If that is too complicated, I am OK with this one. > > > > > > > > I assumed it would be unlikely for a channel to become unwritable before > the > > > > DTLS handshake finished. Is that a bad assumption? Or, are there DTLS > > packets > > > > sent after the handshake finishes? > > > > > > > > Also, even if we cached outgoing packets, we would still want to cache the > > > > CLIENT HELLO in case it's received before the SDP answer which contains > the > > > > remote fingerprint. > > > > > > I don't think you need to worry about that. Caching the client hello solves > > the > > > readable but not writable case, since we don't try to start DTLS until > > writable. > > > > For the unit test, you should be able to set the FakeTransportChannel to > > writable only on the callee side, and then set the caller to writable after > the > > client hello packet is sent. I don't think you need the fake clock. > > The reason a unit test would be difficult is that it would likely involve > "EXPECT_TRUE_WAIT(dtls_done, kSomeTimeout)". But on a slow bot (MSan/ASan/etc.) > the timeout may expire just because the bot is slow, even if things are working > as expected. ... And I'd prefer not to add even more public methods that expose an object's internal state just for the sake of testing.
On 2016/04/26 18:37:48, Taylor Brandstetter wrote: > On 2016/04/26 18:36:16, Taylor Brandstetter wrote: > > On 2016/04/26 17:15:59, juberti2 wrote: > > > On 2016/04/26 05:47:21, juberti2 wrote: > > > > On 2016/04/26 01:32:14, Taylor Brandstetter wrote: > > > > > On 2016/04/25 19:56:32, honghaiz3 wrote: > > > > > > On 2016/04/23 00:34:50, Taylor Brandstetter wrote: > > > > > > > PTAL. > > > > > > > > > > > > > > By the way, I don't have any unit tests because it would be hard to > > > write > > > > > one > > > > > > > that's not flaky, since this is so time-sensitive. > > > > > > > > > > > > > > But I have a CL that adds a simulated clock, which would solve that > > > > problem: > > > > > > > https://codereview.webrtc.org/1895933003/ > > > > > > > > > > > > This will work but I wonder if there is a better way to fix this and > the > > > > other > > > > > > issue Justin mentioned > > > > > > that the DTLS packets may be silently dropped if the underlying > > transport > > > > > > channel is not writable. > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > > The real problem here is that the DTLS channel may be readable but not > > > > > writable. > > > > > > > > > > > > What we are doing here is trying to cache the packet received in this > > > case. > > > > > > Can we instead cache the packets that will be sent out when the > channel > > is > > > > not > > > > > > writable? > > > > > > By doing that, we may fix both issues. > > > > > > If that is too complicated, I am OK with this one. > > > > > > > > > > I assumed it would be unlikely for a channel to become unwritable before > > the > > > > > DTLS handshake finished. Is that a bad assumption? Or, are there DTLS > > > packets > > > > > sent after the handshake finishes? > > > > > > > > > > Also, even if we cached outgoing packets, we would still want to cache > the > > > > > CLIENT HELLO in case it's received before the SDP answer which contains > > the > > > > > remote fingerprint. > > > > > > > > I don't think you need to worry about that. Caching the client hello > solves > > > the > > > > readable but not writable case, since we don't try to start DTLS until > > > writable. > > > > > > For the unit test, you should be able to set the FakeTransportChannel to > > > writable only on the callee side, and then set the caller to writable after > > the > > > client hello packet is sent. I don't think you need the fake clock. > > > > The reason a unit test would be difficult is that it would likely involve > > "EXPECT_TRUE_WAIT(dtls_done, kSomeTimeout)". But on a slow bot > (MSan/ASan/etc.) > > the timeout may expire just because the bot is slow, even if things are > working > > as expected. > > ... And I'd prefer not to add even more public methods that expose an object's > internal state just for the sake of testing. Sorry, I don't follow. Where would we have to expose internal state? I think any fiddling could largely be confined to the fake transport class.
On 2016/04/26 19:35:16, juberti2 wrote: > On 2016/04/26 18:37:48, Taylor Brandstetter wrote: > > On 2016/04/26 18:36:16, Taylor Brandstetter wrote: > > > The reason a unit test would be difficult is that it would likely involve > > > "EXPECT_TRUE_WAIT(dtls_done, kSomeTimeout)". But on a slow bot > > (MSan/ASan/etc.) > > > the timeout may expire just because the bot is slow, even if things are > > working > > > as expected. > > > > ... And I'd prefer not to add even more public methods that expose an object's > > internal state just for the sake of testing. > > Sorry, I don't follow. Where would we have to expose internal state? I think any > fiddling could largely be confined to the fake transport class. The fiddling isn't a problem, it's the test assertion that could be a problem. Though I guess I could just test that the client hello isn't retransmitted and that would be mostly sufficient for now.
Description was changed from ========== Cache a DTLS packet received before the DTLS handshake has started. In some cases, the client DTLS hello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 ========== to ========== Cache a ClientHello received before the DTLS handshake has started. In some cases, the DTLS ClientHello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 ==========
https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:479: if (IsDtlsPacket(data, size)) { On 2016/04/25 20:40:18, juberti2 wrote: > We should probably also verify this is a CLIENT HELLO, at least for logging. Done. https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:592: LOG_J(LS_WARNING, this) << "Discarding cached DTLS packet because " On 2016/04/25 20:40:18, juberti2 wrote: > Log text could be clearer to indicate this is a real malfunction by the remote > side as opposed to just a race condition or some such. Now that the message is changed to "discarding cached ClientHello because we're not the server" is it more clear? https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/1912323002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.h:242: rtc::Buffer cached_dtls_packet_; On 2016/04/25 20:40:19, juberti2 wrote: > Might want to call this cached_client_hello_, if we decide to only do this for > client hellos Done.
lgtm, with comment fixes https://codereview.webrtc.org/1912323002/diff/40001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right): https://codereview.webrtc.org/1912323002/diff/40001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:924: // Epect a DTLS ClientHello to be sent even while client1_ isn't writable. Expect https://codereview.webrtc.org/1912323002/diff/40001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel_unittest.cc:959: // Epect a DTLS ClientHello to be sent even while client1_ doesn't have a Expect
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1912323002/#ps60001 (title: "Fixing typos in comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912323002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912323002/60001
Message was sent while issue was closed.
Description was changed from ========== Cache a ClientHello received before the DTLS handshake has started. In some cases, the DTLS ClientHello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 ========== to ========== Cache a ClientHello received before the DTLS handshake has started. In some cases, the DTLS ClientHello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cache a ClientHello received before the DTLS handshake has started. In some cases, the DTLS ClientHello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 ========== to ========== Cache a ClientHello received before the DTLS handshake has started. In some cases, the DTLS ClientHello may arrive before the server's transport is writable (before it receives a STUN ping response), or even before it receives a remote fingerprint. If this packet is discarded, it may take a second for a it to be sent again. So, this CL caches it instead of dropping it, and feeds it into the SSL library once the handshake has been started. BUG=webrtc:5789 Committed: https://crrev.com/e84cd2eacad78c67b8c31912abc40c14314d4175 Cr-Commit-Position: refs/heads/master@{#12634} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e84cd2eacad78c67b8c31912abc40c14314d4175 Cr-Commit-Position: refs/heads/master@{#12634} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
