Page MenuHomePhabricator

[UBSan] Implement runtime suppressions (PR25066).
ClosedPublic

Authored by samsonov on Dec 8 2015, 4:54 PM.

Details

Summary

Add the ability to suppress UBSan reports for files/functions/modules
at runtime. The user can now pass UBSAN_OPTIONS=suppressions=supp.txt
with the contents of the form:

signed-integer-overflow:file-with-known-overflow.cpp
alignment:function_doing_unaligned_access
vptr:shared_object_with_vptr_failures.so

Suppression categories match the arguments passed to -fsanitize=
flag (although, see below). There is no overhead if suppressions are
not provided. Otherwise there is extra overhead for symbolization.

Limitations:

  1. sometimes suppressions need debug info / symbol table to function properly (although sometimes frontend generates enough info to do the match).
  2. it's only possible to suppress recoverable UB kinds - if you've built the code with -fno-sanitize-recover=undefined, suppressions will not work.
  3. categories are fine-grained check kinds, not groups like "undefined" or "integer", so you can't write "undefined:file_with_ub.cc".

Diff Detail

Repository
rL LLVM

Event Timeline

samsonov updated this revision to Diff 42250.Dec 8 2015, 4:54 PM
samsonov retitled this revision from to [UBSan] Implement runtime suppressions (PR25066)..
samsonov updated this object.
samsonov added reviewers: rsmith, kcc.
samsonov added a subscriber: cfe-commits.

Richard, could you take a look at this?

Kostya, PTAL. IIRC it was your feature request :)

kcc accepted this revision.Dec 16 2015, 5:55 PM
kcc edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Dec 16 2015, 5:55 PM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
compiler-rt/trunk/test/ubsan/TestCases/Integer/suppressions.cpp
1

This test fails on Windows. I tried to debug, but the usual way to run tests seems to not work with compiler-rt:

C:\src\chrome\src\third_party\llvm-bootstrap>python bin\llvm-lit.py ..\llvm\projects\compiler-rt\test\ubsan\TestCases\In
teger\suppressions.cpp -v
llvm-lit.py: discovery.py:113: warning: unable to find test suite for '..\\llvm\\projects\\compiler-rt\\test\\ubsan\\Tes
tCases\\Integer\\suppressions.cpp'
llvm-lit.py: discovery.py:224: warning: input '..\\llvm\\projects\\compiler-rt\\test\\ubsan\\TestCases\\Integer\\suppres
sions.cpp' contained no tests

How do I run individual compiler-rt tests? (They run fine as part of ninja check-all, but that takes very long)

Here's the error (I still don't know how to run single tests through lit, though):

C:\src\chrome\src\third_party\llvm-bootstrap>env UBSAN_OPTIONS=halt_on_error=1:suppressions="C:\src\chrome\src\third_party\llvm-bootstrap\projects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer\Output\suppressions.cpp.tmp.wrong-supp" C:\src\chrome\src\third_party\llvm-bootstrap\projects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer\Output\suppressions.cpp.tmp

C:\src\chrome\src\third_party\llvm-bootstrap>ERROR: expected '='

It's because FlagParser::is_space() considers ':' as a space, so it thinks c:\ ends after the c and then starts parsing another flag at the \.

Fix ideas: Don't treat : as space while in quotes, or use ; as separator on Windows.

And this in turn is because env strips the quotes:

C:\src\chrome\src\third_party\llvm-bootstrap>env UBSAN_OPTIONS=halt_on_error=1:suppressions="C:\src\chrome\src\third_party\llvm-bootstrap\projects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer\Output\suppressions.cpp.tmp.wrong-supp" env

...

UBSAN_OPTIONS=halt_on_error=1:suppressions=C:\src\chrome\src\third_party\llvm-bootstrap\projects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer\Output\suppressions.cpp.tmp.wrong-supp

I've looked at it some, and couldn't find a way of escaping quotes that lit doesn't strip but that ubsan accepts.

'"c:\foo"' gets its quotes stripped by lit, so does """c:\foo""". \"c:\foo\" confuses lit enough that it tries to call lit.util.warning() which doesn't exist.

I've XFAIL'd the test on win32 in 256307 for now. Please take a look when you're back. Maybe teaching lit about \" escaping is the best fix.

Interesting. Do we run test/asan/TestCases/suppressions-interceptor.cc on
Windows? Seems so: I don't see an XFAIL there, and it also uses
suppressions, but with single quote, followed by double quote:

%env_asan_opts=suppressions='"%t.supp"'

Why does it work there?

rnk added a subscriber: rnk.Jan 11 2016, 11:01 AM

Interesting. Do we run test/asan/TestCases/suppressions-interceptor.cc on
Windows? Seems so: I don't see an XFAIL there, and it also uses
suppressions, but with single quote, followed by double quote:

%env_asan_opts=suppressions='"%t.supp"'

Why does it work there?

We need two levels of quoting: one for the shell and one that makes it all the way through to ASan. The single quotes get taken out by ShLexer or bash depending on which is in use, and the inner double quotes escape the colons inside the Windows paths.

This seemed like the most logically consistent thing to me, even if it's ugly. Any better ideas for smuggling colons through ASan options?

This test is failing again when it runs as part of llvm's tests when I
build a clang package for chromium. It passes when I run it in my regular
llvm build directory (where I ran it to test my "fix") -- this explains why
the fix didn't work when I tried it in the chrome case on 12/22. I'm not
sure what's different in the two cases. The path length to the test is
longer, I suppose :-/

Interesting. Do we run test/asan/TestCases/suppressions-interceptor.cc on
Windows? Seems so: I don't see an XFAIL there, and it also uses
suppressions, but with single quote, followed by double quote:

%env_asan_opts=suppressions='"%t.supp"'

Why does it work there?

I figured this part out: We build a 64-bit clang, and since there's no 64-bit asan runtime check-asan isn't part of check-all in that case. Later, we build a 32-bit asan runtime in a separate build dir, but we apparently don't run tests for that build (this looks like an oversight). That's why the asan test doesn't fail -- it doesn't run.

I figured out the difference. If I set up my environment by running setenv.cmd in the windows sdk, the test fails. If I set it up by running vcvarsall.bat, the test succeeds. Not clear yet why. But with the setenv.cmd setup, the asan suppression test fails too.

I re-disabled the test in 257952.

hans added a subscriber: hans.Jan 15 2016, 3:32 PM

I tried to printf debug with this patch:

Index: lib/sanitizer_common/sanitizer_suppressions.cc
===================================================================
--- lib/sanitizer_common/sanitizer_suppressions.cc      (revision 257931)
+++ lib/sanitizer_common/sanitizer_suppressions.cc      (working copy)
@@ -82,6 +82,7 @@
     return false;
   for (uptr i = 0; i < suppressions_.size(); i++) {
     Suppression &cur = suppressions_[i];
+    Printf("matching cur.templ='%s' against str='%s'\n", cur.templ, str);
     if (0 == internal_strcmp(cur.type, type) && TemplateMatch(cur.templ, str))
{
       *s = &cur;
       return true;
@@ -134,6 +135,8 @@
       s.templ[end2 - line] = 0;
       suppressions_.push_back(s);
       has_suppression_type_[type] = true;
+      Printf("suppression type: %s\n", suppression_types_[type]);
+      Printf("template: %s\n", s.templ);
     }
     if (end[0] == 0)
       break;
Index: lib/sanitizer_common/sanitizer_flag_parser.cc
===================================================================
--- lib/sanitizer_common/sanitizer_flag_parser.cc       (revision 257931)
+++ lib/sanitizer_common/sanitizer_flag_parser.cc       (working copy)
@@ -97,6 +97,8 @@
     value = ll_strndup(buf_ + value_start, pos_ - value_start);
   }

+  Printf("parsed flag: '%s' = '%s'\n", name, value);
+
   bool res = run_handler(name, value);
   if (!res) fatal_error("Flag parsing failed.");
 }

Maybe it's failing to symbolize the do_overflow function name? It looks like it's never trying to match it against the suppressions, instead it's trying to match the null string twice for some reason:

Command 7 Stderr:
parsed flag: 'halt_on_error' = '1'
parsed flag: 'suppressions' = 'D:\src\chromium\src\third_party\llvm-bootstrap\pr
ojects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer\Output\suppres
sions.cpp.tmp.func-supp'
suppression type: unsigned-integer-overflow
template: do_overflow
matching cur.templ='do_overflow' against str='D:\src\chromium\src\third_party\ll
vm\projects\compiler-rt\test\ubsan\TestCases\Integer\suppressions.cpp'
matching cur.templ='do_overflow' against str='D:\src\chromium\src\third_party\ll
vm-bootstrap\projects\compiler-rt\test\ubsan\Standalone-x86_64\TestCases\Integer
\Output\suppressions.cpp.tmp'
matching cur.templ='do_overflow' against str='<null>'
matching cur.templ='do_overflow' against str='<null>'
D:\src\chromium\src\third_party\llvm\projects\compiler-rt\test\ubsan\TestCases\I
nteger\suppressions.cpp:25:44: runtime error: unsigned integer overflow: 1000000
0000000000000 + 9000000000000000000 cannot be represented in type 'unsigned long
 long'

After some more debugging, the only thing in this test that's still faiilng on Windows is the "unsigned-integer-overflow:do_overflow" suppression -- when llvm-symbolizer gets symbols from PDBs it currently requires the DIA SDK. If that's not found at cmake time, then llvm-symbolizer silently can't symbolize executables with pdb debug info.

The test should probably have some "REQUIRES: symbols" thingie. (I'll also fix llvm not finding the DIA SDK in our build setup, but the test will still fail for others who don't have that.)