Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556783002:
Fix out of bound reads in ParseIceServerUrl() for various input. (Closed)
DescriptionFix out of bound reads in ParseIceServerUrl() for various input.
BUG=webrtc:6835
Committed: https://crrev.com/bd44bb01843d36b3390fd24561531c5d98031be0
Cr-Commit-Position: refs/heads/master@{#15544}
Patch Set 1 #
Total comments: 2
Patch Set 2 : ParseIceServerUrl(): Clarify invalid transport parameter key error message. #
Total comments: 2
Patch Set 3 : ParseIceServerUrl(): Remove confusing comment to improve readability. #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 ========== to ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 ==========
hnsl@webrtc.org changed reviewers: + pthatcher@webrtc.org
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
lgtm https://codereview.webrtc.org/2556783002/diff/1/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2556783002/diff/1/webrtc/api/peerconnection.cc#... webrtc/api/peerconnection.cc:241: LOG(LS_WARNING) << "Invalid transport param."; nit: Below we use "transport param" to refer to the "udp"/"tcp" string, not the string "transport" itself.
https://codereview.webrtc.org/2556783002/diff/1/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2556783002/diff/1/webrtc/api/peerconnection.cc#... webrtc/api/peerconnection.cc:241: LOG(LS_WARNING) << "Invalid transport param."; On 2016/12/06 21:43:40, Taylor Brandstetter wrote: > nit: Below we use "transport param" to refer to the "udp"/"tcp" string, not the > string "transport" itself. Done.
lgtm
hta@webrtc.org changed reviewers: + hta@webrtc.org
lgtm always another nit :-) https://codereview.webrtc.org/2556783002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2556783002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:244: // As per above grammar transport param will be consist of lower case Drive-by comment: While you're here, can you fix the grammar of the comment? "As per the above grammar, the transport param will consist of lower case letters." Or delete it - I can't see that it aids readability of the code.
https://codereview.webrtc.org/2556783002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2556783002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:244: // As per above grammar transport param will be consist of lower case On 2016/12/09 12:02:09, hta-webrtc wrote: > Drive-by comment: > While you're here, can you fix the grammar of the comment? > > "As per the above grammar, the transport param will consist of lower case > letters." > > Or delete it - I can't see that it aids readability of the code. I have a branch where I refactored the whole parser to make it actually readable. In this patch I opted to just fix this specific bug with as little change as possible. But sure, I'll delete this comment.
The CQ bit was checked by hnsl@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, hta@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2556783002/#ps40001 (title: "ParseIceServerUrl(): Remove confusing comment to improve readability.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481541002656580,
"parent_rev": "9bd24924055651483d7a591461910ec29f6dfb7a", "commit_rev":
"3d0a25e0603a0beeda88327fb8d561108ca8bf2b"}
Message was sent while issue was closed.
Description was changed from ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 ========== to ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 Review-Url: https://codereview.webrtc.org/2556783002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 Review-Url: https://codereview.webrtc.org/2556783002 ========== to ========== Fix out of bound reads in ParseIceServerUrl() for various input. BUG=webrtc:6835 Committed: https://crrev.com/bd44bb01843d36b3390fd24561531c5d98031be0 Cr-Commit-Position: refs/heads/master@{#15544} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bd44bb01843d36b3390fd24561531c5d98031be0 Cr-Commit-Position: refs/heads/master@{#15544} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
