Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issue 1311153003: Add check to prevent TURN usage if the protocol is not allowed. (Closed)

Created:
5 years, 3 months ago by guoweis_webrtc
Modified:
5 years, 3 months ago
Reviewers:
juberti1, pthatcher1
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Base URL:
https://chromium.googlesource.com/external/webrtc@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Add check to prevent TURN usage if the protocol is not allowed. There are 2 ways to design this. 1. TCP Only mode: this means that we disable all UDP protocols across board. 2. disallow TURN over UDP. Along with DISABLE_UDP, DISABLE_STUN, we should achieve the same result. I'm going with #2. BUG=webrtc:4784 R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/13d35f6ffc71c1a649a902369a6fa5637d05d9b0

Patch Set 1 : #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : #

Total comments: 6

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -0 lines) Patch
M webrtc/p2p/base/portallocator.h View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
M webrtc/p2p/client/basicportallocator.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/p2p/client/portallocator_unittest.cc View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (6 generated)
guoweis_webrtc
PTAL. I'm adding the test case.
5 years, 3 months ago (2015-08-25 21:03:29 UTC) #4
pthatcher1
Needs a unit test. https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h#newcode50 webrtc/p2p/base/portallocator.h:50: // IP address. Disallow to ...
5 years, 3 months ago (2015-08-25 22:59:30 UTC) #5
pthatcher1
https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h#newcode31 webrtc/p2p/base/portallocator.h:31: PORTALLOCATOR_DISABLE_UDP = 0x01, Can you add a comment here ...
5 years, 3 months ago (2015-08-25 23:12:00 UTC) #6
guoweis_webrtc
On 2015/08/25 23:12:00, pthatcher1 wrote: > https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h > File webrtc/p2p/base/portallocator.h (right): > > https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portallocator.h#newcode31 > ...
5 years, 3 months ago (2015-08-26 06:10:14 UTC) #7
pthatcher1
lgtm, assuming you fix the comments https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/base/portallocator.h#newcode31 webrtc/p2p/base/portallocator.h:31: // Disable local ...
5 years, 3 months ago (2015-08-26 17:27:53 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311153003/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311153003/90001
5 years, 3 months ago (2015-08-26 20:30:34 UTC) #11
guoweis_webrtc
fixed all comments. landing it now. https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portallocator_unittest.cc File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portallocator_unittest.cc#newcode620 webrtc/p2p/client/portallocator_unittest.cc:620: cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); On 2015/08/26 ...
5 years, 3 months ago (2015-08-26 20:31:37 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
5 years, 3 months ago (2015-08-26 22:30:42 UTC) #14
guoweis_webrtc
Committed patchset #4 (id:90001) manually as 13d35f6ffc71c1a649a902369a6fa5637d05d9b0 (presubmit successful).
5 years, 3 months ago (2015-08-26 22:33:06 UTC) #15
juberti1
I'm not sure I understand this. Why doesn't the existing DISABLE_UDP flag work here?
5 years, 3 months ago (2015-08-26 22:48:44 UTC) #16
pthatcher2
5 years, 3 months ago (2015-08-26 22:53:15 UTC) #17
Message was sent while issue was closed.
On 2015/08/26 22:48:44, juberti1 wrote:
> I'm not sure I understand this. Why doesn't the existing DISABLE_UDP flag work
> here?

DISABLE_UDP is used to control the allocation phases, here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...


If we change the meaning to include UDP to TURN servers, and then used it for
this purpose, I believe that code would break.

I suppose we could refactor that code and then use DISABLE_UDP to mean "disable
all outgoing UDP", but then we'd have to change DISABLE_TCP to be consistent as
well.  I think it would end up being a bit of a rat hole.

Powered by Google App Engine
This is Rietveld 408576698