sanitizers: actually enable UBSAN by edwintorok · Pull Request #13293 · ocaml/ocaml

edwintorok · 2024-07-12T22:36:29Z

While working on #13290 (comment) I noticed that UBSAN is not active.

Using -fsanitize-trap is a trap: it doesn't actually do anything on its own (I would've expected it to at least warn me, but it doesn't).
The latest https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#usage documents this:

-fsanitize-trap= and -fsanitize-recover= are a no-op in the absence of a -fsanitize= option. There is no unused
command line option warning.

The fix is simple: use -fsanitize=$ubsan -fsanitize-trap=$ubsan instead of just -fsanitize-trap=$ubsan.

Interestingly it was then complaining that -O0 is not valid for -fsanitize=object-size (this extra message would cause a testsuite failure), and the only way to fix that is to put an -O1 into LDFLAGS.

To avoid falling into similar traps I've added 2 small tests in the CI script itself, which compiles 2 known buggy C programs, and checks whether the sanitizers find the expected bug (fails early otherwise).

I've also replaced -fsanitize-trap with -fno-sanitize-recover=all and UBSAN_OPTIONS=print_stacktrace=1. This should make UBSAN work a bit like ASAN, I think in the CI a stacktrace is better than a coredump.
If a coredump is desired for local debugging then the script can be changed to UBSAN_OPTIONS="abort_on_error=1".

Successful run in Inria's CI: https://ci.inria.fr/ocaml/job/precheck-sanitizers/2/consoleText
(sorry I couldn't figure how to create a user, so created/launched it as anonymous user)

This PR probably doesn't need a Changes entry, it only changes the CI.

edwintorok · 2024-07-12T22:37:06Z

I'd suggest to also backport this to the 5.2 and 4.14 branches, I can open PRs for that if desired once this one is reviewed/merged.

edwintorok · 2024-07-12T22:46:17Z

I notice there is an earlier PR that modifies this script too.
I can rebase this PR once that one is merged: #13219 (comment)

shindere · 2024-07-19T07:34:22Z

Thanks a lot for the work and the very clear and readable explanations! #13219 just got merged so it should be possible to rebase. May I please aksk to send a comment here when done?

edwintorok · 2024-07-19T13:40:47Z

Rebased and test passed in the Inria CI: https://ci.inria.fr/ocaml/job/precheck-sanitizers/6/

This is a pure CI PR, so I think no-change-entry-needed is also needed as a label.

For reference here is how a ubsan error report looks like from the CI logs (stacktraces are working and you get file:line information):

ubsan.c:9:10: runtime error: load of value 100, which is not a valid value for type 'bool'
    #0 0x562e30b612e4 in main /home/barsac/ci/builds/workspace/precheck-sanitizers/ubsan.c:9:10
    #1 0x7fe582437d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #2 0x7fe582437e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #3 0x562e30a82304 in _start (/home/barsac/ci/builds/workspace/precheck-sanitizers/ubsan+0x2b304) (BuildId: c1a9bafaaf4c0db840c1a9ed574dc73f59d5fa9c)

And here is how an ASAN error report looks like:

=================================================================
==3661671==ERROR: AddressSanitizer: attempting double-free on 0x502000000010 in thread T0:
    #0 0x55774e5895a6 in free (/home/barsac/ci/builds/workspace/precheck-sanitizers/asan+0xcd5a6) (BuildId: 8db9571a5c145b307f9cf869e835211c190a6f4f)
    #1 0x55774e5c62f5 in main /home/barsac/ci/builds/workspace/precheck-sanitizers/asan.c:6:3
    #2 0x7f7a16a9bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #3 0x7f7a16a9be3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #4 0x55774e4e7304 in _start (/home/barsac/ci/builds/workspace/precheck-sanitizers/asan+0x2b304) (BuildId: 8db9571a5c145b307f9cf869e835211c190a6f4f)

0x502000000010 is located 0 bytes inside of 4-byte region [0x502000000010,0x502000000014)
freed by thread T0 here:
    #0 0x55774e5895a6 in free (/home/barsac/ci/builds/workspace/precheck-sanitizers/asan+0xcd5a6) (BuildId: 8db9571a5c145b307f9cf869e835211c190a6f4f)
    #1 0x55774e5c62ed in main /home/barsac/ci/builds/workspace/precheck-sanitizers/asan.c:5:3
    #2 0x7f7a16a9bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

previously allocated by thread T0 here:
    #0 0x55774e589855 in malloc (/home/barsac/ci/builds/workspace/precheck-sanitizers/asan+0xcd855) (BuildId: 8db9571a5c145b307f9cf869e835211c190a6f4f)
    #1 0x55774e5c62e2 in main /home/barsac/ci/builds/workspace/precheck-sanitizers/asan.c:4:13
    #2 0x7f7a16a9bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: double-free (/home/barsac/ci/builds/workspace/precheck-sanitizers/asan+0xcd5a6) (BuildId: 8db9571a5c145b307f9cf869e835211c190a6f4f) in free
==3661671==ABORTING

shindere · 2024-07-19T14:12:41Z

Török Edwin (2024/07/19 06:41 -0700):

Rebased and test passed in the Inria CI: https://ci.inria.fr/ocaml/job/precheck-sanitizers/6/

Thanks!

This is a pure CI PR, so I think `no-change-entry-needed` is also needed as a label.

I did add the label, I think it would not be unfair to add a Changes entry for this PR, just for the sake of crediting its author. As you prefer. Overall I very much like this PR, both what it does and how it does it (in a series of super easy to review commits, with all the explanations needed). I have one nit pick and two very minor quesitons. Nitpick: would youmind ensuring thatall the commit messages start with an uppercase letter, please? Not that I am proud of this obsession, but apparently I have it. Re: you commit: `sanitizers: add a test that ASAN/UBSAN works` I was wondering whether one wouldn't want to remove all the files after the test has been completed? I was also a bit uncomfortable with the use of singular and plural in the commit title. Perhaps I'd have felt better with Sanitizers: add a test that ASAN/UBSAN work (no s) for isntance... Also, re:the commit `sanitizers: add -g flag`, I was unsure if `-g` shouldn't be added to `LDFLAGS`, too, but I guess it's not super important since the next commit adds `-Og` to both `CFLAGS` and `LDFLAGS`. Thanks again for your great work!

The clang-14 docs are not clear about this, but the latest docs say: > -fsanitize-trap= and -fsanitize-recover= are a no-op in the absence of a -fsanitize= option. There is no unused command line option warning. Avoid this trap and specify `-fsanitize=$ubsan` too. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Apparently >-O0 needs to be supplied when linking too: ``` clang: warning: the object size sanitizer has no effect at -O0, but is explicitly enabled: -fsanitize=bool,builtin,bounds,enum,nonnull-attribute,nullability,object-size,pointer-overflow,returns-nonnull-attribute,shift-exponent,unreachable [-Winvalid-command-line-argument] ``` Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Move compiler flag definitions to shell variables. This will enable testing them. No functional change yet. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Compile 2 small test programs, and check expected output. The test programs are deleted on success. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Still stop on first error, but instead of dumping core print a message, and stacktrace, like ASAN. This may be more convenient when used in a CI than a coredump. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

For better stacktraces. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

This is currently equivalent to -O1 on clang, but may improve debuggability in the future compared to -O1. GCC already disables certain optimizations at -Og, and recommends it when using the sanitizers: ``` To get more accurate stack traces, it is possible to use options such as -O0, -O1, or -Og (which, for instance, prevent most function inlining) ``` (there are also other flag recommendations, but I think they are only relevant if you want to use -O2 or higher, they wouldn't be active at -O1/-Og) Also don't use -O0. Even for pure debugging, the manual recommends -Og: ``` It is a better choice than -O0 for producing debuggable code because some compiler passes that collect debug information are disabled at -O0 ``` Signed-off-by: Edwin Török <edwin.torok@cloud.com>

edwintorok · 2024-07-19T15:02:13Z

I did add the label, I think it would not be unfair to add a Changes
entry for this PR, just for the sake of crediting its author. As you
prefer.

Thanks, adding a Changes entry has a tendency to conflict,
especially if there are multiple PRs in flight.
Lets keep it simple for now, as this is not a user visible change.

I have one nit pick and two very minor quesitons.

I updated the commit messages to match your suggestions.

Re: you commit: sanitizers: add a test that ASAN/UBSAN works

I was wondering whether one wouldn't want to remove all the files after
the test has been completed?

I've added an rm -f ubsan ubsan.o ubsan.c (and equivalent for asan).
Checked that it still passes in the CI https://ci.inria.fr/ocaml/job/precheck-sanitizers/8/

Also, re:the commit sanitizers: add -g flag, I was unsure if -g shouldn't be added to LDFLAGS, too

I think that -g in LDFLAGS only matters if you do LTO. Some compilers now do LTO by default (e.g. latest GCC when invoked by rpmbuild), so I've added the flag to future proof this.

shindere · 2024-07-19T15:27:52Z

Many thanks for your prompt andgood willing repsonse! I will approve now and merge as soon as CI is done.

edwintorok · 2024-07-19T17:04:48Z

Thanks for the review! (I think I have a few more sanitizer related PRs, I'll update those later).

shindere · 2024-07-19T21:28:11Z

Török Edwin (2024/07/19 10:05 -0700):

Thanks for the review! (I think I have a few more sanitizer related PRs, I'll update those later).

You're welcome, and happy to review other PRs along these lines.

edwintorok mentioned this pull request Jul 12, 2024

Let CI's sanitizer job use clang's default version #13219

Merged

edwintorok mentioned this pull request Jul 14, 2024

Fix uninitialized read in runtime_events and toplevel bigarray printing. Document how to use MSAN. #13290

Closed

edwintorok force-pushed the private/edvint/really-ubsan branch from 5bc4b9c to 814c007 Compare July 19, 2024 13:14

shindere added the no-change-entry-needed label Jul 19, 2024

edwintorok added 6 commits July 19, 2024 15:51

Sanitizers: move flag definitions

c3b3464

Move compiler flag definitions to shell variables. This will enable testing them. No functional change yet. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Sanitizers: add a test that ASAN/UBSAN work

52d93a5

Compile 2 small test programs, and check expected output. The test programs are deleted on success. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Sanitizer(UBSAN): switch to printing instead of trap

9f7aad8

Still stop on first error, but instead of dumping core print a message, and stacktrace, like ASAN. This may be more convenient when used in a CI than a coredump. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Sanitizers: add -g flag

9addfad

For better stacktraces. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

edwintorok force-pushed the private/edvint/really-ubsan branch from 814c007 to 7c08154 Compare July 19, 2024 14:55

edwintorok force-pushed the private/edvint/really-ubsan branch from 7c08154 to 307f1e1 Compare July 19, 2024 14:57

shindere approved these changes Jul 19, 2024

View reviewed changes

shindere merged commit bd63570 into ocaml:trunk Jul 19, 2024

edwintorok deleted the private/edvint/really-ubsan branch July 19, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sanitizers: actually enable UBSAN#13293

sanitizers: actually enable UBSAN#13293
shindere merged 7 commits intoocaml:trunkfrom
edwintorok:private/edvint/really-ubsan

edwintorok commented Jul 12, 2024

Uh oh!

edwintorok commented Jul 12, 2024

Uh oh!

edwintorok commented Jul 12, 2024

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024 •

edited

Loading

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024 •

edited

Loading

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

edwintorok commented Jul 12, 2024

Uh oh!

edwintorok commented Jul 12, 2024

Uh oh!

edwintorok commented Jul 12, 2024

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

edwintorok commented Jul 19, 2024

Uh oh!

shindere commented Jul 19, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

edwintorok commented Jul 19, 2024 •

edited

Loading

edwintorok commented Jul 19, 2024 •

edited

Loading