Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318653003:
Fixing stack buffer overflow (read) in SctpDataEngine. (Closed)
|
Created:
4 years, 3 months ago by Taylor Brandstetter Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing stack buffer overflow (read) in SctpDataEngine.
Was using the wrong size when memcpy'ing a sockaddr_conn.
BUG=chromium:642638
TBR=pthatcher@webrtc.org
Committed: https://crrev.com/a4d40cb502558c27c82303bb98aedc2dd8d6a888
Cr-Commit-Position: refs/heads/master@{#14111}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Getting rid of unneeded cast. #Messages
Total messages: 18 (9 generated)
skvlad@webrtc.org changed reviewers: + skvlad@webrtc.org
lgtm https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:553: memcpy(reinterpret_cast<sockaddr_conn*>(¶ms.spp_address), &remote_sconn, Could this be done as an assignment instead? That would make it just a bit less likely to use the wrong size. *reinterpret_cast<sockaddr_conn*>(¶ms.spp_address) = remote_sconn;
https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:553: memcpy(reinterpret_cast<sockaddr_conn*>(¶ms.spp_address), &remote_sconn, On 2016/09/06 18:29:16, skvlad wrote: > Could this be done as an assignment instead? That would make it just a bit less > likely to use the wrong size. > *reinterpret_cast<sockaddr_conn*>(¶ms.spp_address) = remote_sconn; Hmm. It seems like that would work, but I see the memcpy pattern used with sockaddrs pretty consistently elsewhere in the codebase. Maybe there's a caveat we're not thinking of.
honghaiz@webrtc.org changed reviewers: + honghaiz@webrtc.org
lgtm https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:553: memcpy(reinterpret_cast<sockaddr_conn*>(¶ms.spp_address), &remote_sconn, Does it matter to reinterpret_cast the dest pointer? memcpy takes arguments (void*, void*, size_t).
https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/2318653003/diff/1/webrtc/media/sctp/sctpdataeng... webrtc/media/sctp/sctpdataengine.cc:553: memcpy(reinterpret_cast<sockaddr_conn*>(¶ms.spp_address), &remote_sconn, On 2016/09/06 22:12:09, honghaiz3 wrote: > Does it matter to reinterpret_cast the dest pointer? > memcpy takes arguments (void*, void*, size_t). Maybe not; again, I was copying the pattern used elsewhere. I'll change this.
Description was changed from ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 ========== to ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 TBR=pthatcher@webrtc.org ==========
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org - honghaiz@webrtc.org, skvlad@webrtc.org
pthatcher: Need you to review since none of us are webrtc/media owners.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org, skvlad@webrtc.org Link to the patchset: https://codereview.webrtc.org/2318653003/#ps20001 (title: "Getting rid of unneeded cast.")
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
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 TBR=pthatcher@webrtc.org ========== to ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 TBR=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/a4d40cb502558c27c82303bb9... ==========
Message was sent while issue was closed.
Description was changed from ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 TBR=pthatcher@webrtc.org ========== to ========== Fixing stack buffer overflow (read) in SctpDataEngine. Was using the wrong size when memcpy'ing a sockaddr_conn. BUG=chromium:642638 TBR=pthatcher@webrtc.org Committed: https://crrev.com/a4d40cb502558c27c82303bb98aedc2dd8d6a888 Cr-Commit-Position: refs/heads/master@{#14111} ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as a4d40cb502558c27c82303bb98aedc2dd8d6a888 (presubmit successful).
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a4d40cb502558c27c82303bb98aedc2dd8d6a888 Cr-Commit-Position: refs/heads/master@{#14111} |
