|
|
Created:
3 years, 4 months ago by oprypin_webrtc Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix a delete type mismatch (deleting a sockaddr_in6* as a sockaddr*)
This triggered ASan, saying "object passed to delete has wrong type".
This error is caused by allocating a `struct sockaddr_in6`, casting it
and deleting it as `struct sockaddr*` which has a smaller size.
BUG=None
Review-Url: https://codereview.webrtc.org/2999053002
Cr-Commit-Position: refs/heads/master@{#19401}
Committed: https://chromium.googlesource.com/external/webrtc/+/1ea631f4a769305a372cfb8fcf455c10f8f0a8aa
Patch Set 1 #Patch Set 2 : Rework to use malloc/free #
Total comments: 4
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Fix a delete type mismatch (deleting a sockaddr_in6* as a sockaddr*) This triggered ASan, saying "object passed to delete has wrong type". This error is caused by allocating a `struct sockaddr_in6`, casting it and deleting it as `struct sockaddr*` which has a smaller size. BUG=None ========== to ========== Fix a delete type mismatch (deleting a sockaddr_in6* as a sockaddr*) This triggered ASan, saying "object passed to delete has wrong type". This error is caused by allocating a `struct sockaddr_in6`, casting it and deleting it as `struct sockaddr*` which has a smaller size. BUG=None ==========
oprypin@webrtc.org changed reviewers: + honghaiz@webrtc.org
oprypin@webrtc.org changed reviewers: + kwiberg@webrtc.org
Yeah, I guess this works, but this basically a C data structure that you're manipulating, right? So another way would be to go the "idiomatic C" route and use malloc+free instead of new+delete, and then you wouldn't have this problem because malloc and free deal with untyped pointers. I won't object if you like the present solution better, though, since it *is* a smaller change. So lgtm.
The CQ bit was checked by oprypin@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/...
On 2017/08/17 21:17:35, kwiberg-webrtc wrote: > Yeah, I guess this works, but this basically a C data structure that you're > manipulating, right? So another way would be to go the "idiomatic C" route and > use malloc+free instead of new+delete, and then you wouldn't have this problem > because malloc and free deal with untyped pointers. > > I won't object if you like the present solution better, though, since it *is* a > smaller change. So lgtm. Thanks. I changed to use malloc+free. The change is smaller now.
lgtm https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... File webrtc/rtc_base/network_unittest.cc (right): https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... webrtc/rtc_base/network_unittest.cc:129: malloc(sizeof(struct sockaddr_in6))); Note: Unlike in C, in C++ you don't have to say "struct Foo" everywhere. Just "Foo" will do. https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... webrtc/rtc_base/network_unittest.cc:130: memset(ipv6_addr, 0, sizeof(struct sockaddr_in6)); You could replace the malloc+memset pair with calloc if you wanted: auto* ipv6_addr = static_cast<sockaddr_in6*>(calloc(1, sizeof(sockaddr_in6)));
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... File webrtc/rtc_base/network_unittest.cc (right): https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... webrtc/rtc_base/network_unittest.cc:129: malloc(sizeof(struct sockaddr_in6))); On 2017/08/17 22:18:48, kwiberg-webrtc wrote: > Note: Unlike in C, in C++ you don't have to say "struct Foo" everywhere. Just > "Foo" will do. Leaving as is because these pure C structs are referred to with `struct` throughout the file. https://codereview.webrtc.org/2999053002/diff/20001/webrtc/rtc_base/network_u... webrtc/rtc_base/network_unittest.cc:130: memset(ipv6_addr, 0, sizeof(struct sockaddr_in6)); On 2017/08/17 22:18:48, kwiberg-webrtc wrote: > You could replace the malloc+memset pair with calloc if you wanted: > > auto* ipv6_addr = static_cast<sockaddr_in6*>(calloc(1, sizeof(sockaddr_in6))); I did give `calloc` a fair consideration, but it's really uncommon in our codebase, even in cases like this one. Leaving.
The CQ bit was checked by oprypin@webrtc.org
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": 20001, "attempt_start_ts": 1503040369369030, "parent_rev": "9e0c742f1b564126aec0071f2c93d2e912889764", "commit_rev": "1ea631f4a769305a372cfb8fcf455c10f8f0a8aa"}
Message was sent while issue was closed.
Description was changed from ========== Fix a delete type mismatch (deleting a sockaddr_in6* as a sockaddr*) This triggered ASan, saying "object passed to delete has wrong type". This error is caused by allocating a `struct sockaddr_in6`, casting it and deleting it as `struct sockaddr*` which has a smaller size. BUG=None ========== to ========== Fix a delete type mismatch (deleting a sockaddr_in6* as a sockaddr*) This triggered ASan, saying "object passed to delete has wrong type". This error is caused by allocating a `struct sockaddr_in6`, casting it and deleting it as `struct sockaddr*` which has a smaller size. BUG=None Review-Url: https://codereview.webrtc.org/2999053002 Cr-Commit-Position: refs/heads/master@{#19401} Committed: https://chromium.googlesource.com/external/webrtc/+/1ea631f4a769305a372cfb8fc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/1ea631f4a769305a372cfb8fc... |