Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3005273002:
Add a check that sanitizers cause a fatal failure (Closed)
|
Created:
3 years, 3 months ago by oprypin_webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd a check that sanitizers cause a fatal failure
Inspired by https://chromium.googlesource.com/chromium/src/+/master/base/tools_sanity_unittest.cc
BUG=webrtc:8214
R=kjellander@webrtc.org, kwiberg@webrtc.org
Review-Url: https://codereview.webrtc.org/3005273002 .
Cr-Commit-Position: refs/heads/master@{#19830}
Committed: https://webrtc.googlesource.com/src/+/66ca7e3b06aeb3b2804596914e1f6313d9bbe98d
Patch Set 1 : Add a check that sanitizers cause a fatal failure #Patch Set 2 : Rework #
Total comments: 15
Patch Set 3 : Fix msan check #Patch Set 4 : Minor fixes #Patch Set 5 : Include fix #Patch Set 6 : Rework ubsan_vptr check #
Total comments: 3
Patch Set 7 : Rebase #Messages
Total messages: 51 (37 generated)
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/...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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/...
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/23719)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add a check that sanitizers cause a fatal failure Inspired by https://chromium.googlesource.com/chromium/src/+/master/base/tools_sanity_uni... BUG=webrtc:8214 ========== to ========== Add a check that sanitizers cause a fatal failure Inspired by https://chromium.googlesource.com/chromium/src/+/master/base/tools_sanity_uni... BUG=webrtc:8214 ==========
oprypin@webrtc.org changed reviewers: + kjellander@webrtc.org, kwiberg@webrtc.org
Patchset #2 (id:80001) has been deleted
lgtm, but I'd like a solid C++ programmer like Karl to look at the code as well.
https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/BUILD.gn File webrtc/rtc_tools/BUILD.gn (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/BUILD.g... webrtc/rtc_tools/BUILD.gn:323: "../rtc_base:rtc_base", The :rtc_base suffix is unnecessary, right? https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:22: #if defined(MEMORY_SANITIZER) There are preprocessor symbols here [https://chromium.googlesource.com/external/webrtc/+/HEAD/webrtc/rtc_base/sanitizer.h#16] that are set to 0 or 1. Are the ones you're using here better somehow? https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:32: UseOfUninitializedValue(); Hmm. Why don't we expect to die here? https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:71: EXPECT_DEATH({ SignedIntegerOverflow(); InvalidVptr(); }, Why are you doing two lethal things here? https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:80: : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())), rtc::MakeUnique<NullSocketServer>()
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/...
https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/BUILD.gn File webrtc/rtc_tools/BUILD.gn (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/BUILD.g... webrtc/rtc_tools/BUILD.gn:323: "../rtc_base:rtc_base", On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > The :rtc_base suffix is unnecessary, right? Oops. I think I just copied GN's suggestion when it was missing. Done. https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:22: #if defined(MEMORY_SANITIZER) On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > There are preprocessor symbols here > [https://chromium.googlesource.com/external/webrtc/+/HEAD/webrtc/rtc_base/sanitizer.h#16] > that are set to 0 or 1. Are the ones you're using here better somehow? I didn't know about these. I got my example from https://codereview.webrtc.org/3009123002/patch/160001/170010 But there are 4 kinds of sanitizer macros, while the ones you're suggesting are only 2. https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:32: UseOfUninitializedValue(); On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > Hmm. Why don't we expect to die here? Hmm indeed. It was incorrect. Had to rework this. https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:71: EXPECT_DEATH({ SignedIntegerOverflow(); InvalidVptr(); }, On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > Why are you doing two lethal things here? ubsan fails on only one of them, ubsan_vptr only fails on the other one, and I can't discern which one is currently running. https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:80: : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())), On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > rtc::MakeUnique<NullSocketServer>() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/25997)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:22: #if defined(MEMORY_SANITIZER) On 2017/09/13 14:48:15, oprypin_webrtc wrote: > On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > > There are preprocessor symbols here > > > [https://chromium.googlesource.com/external/webrtc/+/HEAD/webrtc/rtc_base/sanitizer.h#16] > > that are set to 0 or 1. Are the ones you're using here better somehow? > > I didn't know about these. I got my example from > https://codereview.webrtc.org/3009123002/patch/160001/170010 > But there are 4 kinds of sanitizer macros, while the ones you're suggesting are > only 2. OK, FOO_SANITIZER seems to be set by Chromium's GN files. Using our own macros from sanitizer.h seems generally safer and cleaner. It'd be great if you could add the missing ones to sanitizer.h. (Alternatively, just keep using Chromium's macros here, since it looks like I'll have to sweep the tree clean of all of these anyway...) https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: }; IIUC, you don't need the subclass or a virtual destructor for ubsan_vptr to trigger. You should just need to call virtual function f() on a deleted object. Alternatively---or additionally---keep the current hierarchy, and do something like Base b; auto* d = static_cast<Derived*>(&b); // bad down cast d->f(); // virtual function call with object of wrong dynamic type ("-fsanitize=vptr: Use of an object whose vptr indicates that it is of the wrong dynamic type, or that its lifetime has not begun or has ended. ") https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:71: EXPECT_DEATH({ SignedIntegerOverflow(); InvalidVptr(); }, On 2017/09/13 14:48:15, oprypin_webrtc wrote: > On 2017/09/13 11:59:23, kwiberg-webrtc wrote: > > Why are you doing two lethal things here? > > ubsan fails on only one of them, ubsan_vptr only fails on the other one, and I > can't discern which one is currently running. Hmm, yeah, I see. And it looks like it'll be messy to make GN tell us if it's on or not. Problem is, this test only proves that both ubsan and ubsan_vptr fail on at least one of the tests. :-(
https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: }; On 2017/09/14 02:15:02, kwiberg-webrtc wrote: > IIUC, you don't need the subclass or a virtual destructor for ubsan_vptr to > trigger. You should just need to call virtual function f() on a deleted object. > > Alternatively---or additionally---keep the current hierarchy, and do something > like > > Base b; > auto* d = static_cast<Derived*>(&b); // bad down cast > d->f(); // virtual function call with object of wrong dynamic type > > ("-fsanitize=vptr: Use of an object whose vptr indicates that it is of the > wrong dynamic type, or that its lifetime has not begun or has ended. ") OK, I used the alternative implementation. It's nicer not to rely on a null pointer.
lgtm now. Too bad we couldn't figure out a way to differentiate between ubsan and ubsan_vptr, though. https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/100001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: }; On 2017/09/14 06:48:33, oprypin_webrtc wrote: > On 2017/09/14 02:15:02, kwiberg-webrtc wrote: > > IIUC, you don't need the subclass or a virtual destructor for ubsan_vptr to > > trigger. You should just need to call virtual function f() on a deleted > object. > > > > Alternatively---or additionally---keep the current hierarchy, and do something > > like > > > > Base b; > > auto* d = static_cast<Derived*>(&b); // bad down cast > > d->f(); // virtual function call with object of wrong dynamic type > > > > ("-fsanitize=vptr: Use of an object whose vptr indicates that it is of the > > wrong dynamic type, or that its lifetime has not begun or has ended. ") > > OK, I used the alternative implementation. It's nicer not to rely on a null > pointer. Acknowledged. https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: virtual ~Base() {} Nit: You don't need the virtual destructor. Normally it's good manners to have one whenever you have virtual methods, but in this case it probably makes more sense to provide a minimal test case.
https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: virtual ~Base() {} On 2017/09/14 08:23:22, kwiberg-webrtc wrote: > Nit: You don't need the virtual destructor. Normally it's good manners to have > one whenever you have virtual methods, but in this case it probably makes more > sense to provide a minimal test case. error: 'rtc::(anonymous namespace)::Base' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/3005273002/#ps180001 (title: "Rework ubsan_vptr check")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... File webrtc/rtc_tools/sanitizers_unittest.cc (right): https://codereview.webrtc.org/3005273002/diff/180001/webrtc/rtc_tools/sanitiz... webrtc/rtc_tools/sanitizers_unittest.cc:62: virtual ~Base() {} On 2017/09/14 08:33:04, oprypin_webrtc wrote: > On 2017/09/14 08:23:22, kwiberg-webrtc wrote: > > Nit: You don't need the virtual destructor. Normally it's good manners to have > > one whenever you have virtual methods, but in this case it probably makes more > > sense to provide a minimal test case. > > error: 'rtc::(anonymous namespace)::Base' has virtual functions but non-virtual > destructor [-Werror,-Wnon-virtual-dtor] Oh, right, we have a machine that checks that for us. Well then, we mustn't make the machines cranky---carry on!
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1505377992243280,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"21f224669f889517b70e7e450cb4e1d4ed339d35"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1505377992243280,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"21f224669f889517b70e7e450cb4e1d4ed339d35"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1505377992243280,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"21f224669f889517b70e7e450cb4e1d4ed339d35"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/3005273002/#ps200001 (title: "Rebase")
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": 200001, "attempt_start_ts": 1505380402284900,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"e97d216f568a1314ddbd001d5568de28857de7a2"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1505380402284900,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"e97d216f568a1314ddbd001d5568de28857de7a2"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1505380402284900,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"e97d216f568a1314ddbd001d5568de28857de7a2"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
Description was changed from ========== Add a check that sanitizers cause a fatal failure Inspired by https://chromium.googlesource.com/chromium/src/+/master/base/tools_sanity_uni... BUG=webrtc:8214 ========== to ========== Add a check that sanitizers cause a fatal failure Inspired by https://chromium.googlesource.com/chromium/src/+/master/base/tools_sanity_uni... BUG=webrtc:8214 R=kjellander@webrtc.org, kwiberg@webrtc.org Review-Url: https://codereview.webrtc.org/3005273002 . Cr-Commit-Position: refs/heads/master@{#19830} Committed: https://webrtc.googlesource.com/src/+/66ca7e3b06aeb3b2804596914e1f6313d9bbe98d ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) manually as 66ca7e3b06aeb3b2804596914e1f6313d9bbe98d (presubmit successful). |
