Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add druntime support for AddressSanitizer stack-use-after-return, fakestack #3888

Merged
merged 5 commits into from Mar 4, 2022

Conversation

JohanEngelen
Copy link
Member

See https://github.com/google/sanitizers/wiki/AddressSanitizerExampleUseAfterReturn for an explanation of what bugs can be caught with ASan's fakestack enabled.

Apologies for the mess in git on master :(

@JohanEngelen
Copy link
Member Author

The problem is that this change (as well as the previous ASan support addition to druntime) creates an ABI incompatibility between druntime with/without version SupportSanitizers: it adds variables to structs/classes that are exposed to the userand that are allocated by user code.

I see two options to solve this:

  1. Keep the ABI incompatibility. Require ALL d-source to be recompiled with -d-version=SupportSanitizers.
  2. Make the ABI fully compatible. We can do this by unconditionally adding needed variables to the structs/classes. This leads to a little bit of overhead (small), but it is useless for 99.99% of the time (not many people using ASan). The extra instructions can still be versioned out, so the only overhead is in increasing the size of a few aggregates.

I prefer option 2. With extra effort, I think I can reduce the amount of storage needed (e.g. don't store fakestack in each StackContext.)

@kinke
Copy link
Member

kinke commented Jan 21, 2022

The extra instructions can still be versioned out

Are they in the druntime lib only, not instantiated? I.e., would user code not have to be compiled with -d-version=SupportSanitizers if all druntime versions had the required extra fields? And so one could easily switch out the static/shared druntime when linking to benefit from sanitizers?

If that's the case, then maybe druntime could use 2 internal versions - SupportSanitizers for extra fields etc. and enabled by default, and UseSanitizers or so to enable the extra instructions for sanitizer-enabled druntime libs. Just for the paranoids on systems with very limited resources.
Edit: Argh no, one version cannot be druntime-internal (needed for user invocations too), the one defining the fields. I'd say then either ignore it (unconditionally add the fields), or use an inverted NoSanitizers version for the paranoids.

@JohanEngelen
Copy link
Member Author

Remaining error on Cirrus CI / Ubuntu 18.04 x64 multilib rtSanitizers:

$ ":" "RUN: at line 9"
$ "/tmp/build/bin/ldc2" "-g" "-fsanitize=address" "/tmp/cirrus-ci-build/tests/sanitizers/asan_use_after_return.d" "-of=/tmp/build/tests/sanitizers/Output/asan_use_after_return.d.tmp"
$ ":" "RUN: at line 10"
$ "not" "env" "env" "ASAN_OPTIONS=abort_on_error=0:log_to_syslog=0:detect_stack_use_after_return=true" "/tmp/build/tests/sanitizers/Output/asan_use_after_return.d.tmp"
$ "FileCheck" "/tmp/cirrus-ci-build/tests/sanitizers/asan_use_after_return.d"
# command stderr:
/tmp/cirrus-ci-build/tests/sanitizers/asan_use_after_return.d:31:16: error: expected string not found in input
// CHECK-NEXT: #1 {{.*}} in {{.*}}asan_use_after_return.d:[[@LINE+1]]
               ^
<stdin>:4:146: note: scanning from here
 #0 0x55bd33be650e in _D21asan_use_after_return1fFNiiZ12__dgliteral2MFNaNbNiNfZv /tmp/cirrus-ci-build/tests/sanitizers/asan_use_after_return.d:23:21
                                                                                                                                                 ^
<stdin>:4:146: note: with expression "@LINE+1" equal to "32"
 #0 0x55bd33be650e in _D21asan_use_after_return1fFNiiZ12__dgliteral2MFNaNbNiNfZv /tmp/cirrus-ci-build/tests/sanitizers/asan_use_after_return.d:23:21
                                                                                                                                                 ^
<stdin>:5:12: note: possible intended match here
 #1 0x55bd33be650e in _D21asan_use_after_return1fFNiiZ12__dgliteral2MFNaNbNiNfZv (/tmp/build/tests/sanitizers/Output/asan_use_after_return.d.tmp+0x1e50e)
           ^

This is surprising. Somehow there is debug line information for the first check on #0, but there is no line info for #1.
@kinke Have you seen this "sporadic" debug line info more often in testing? I'm thinking about just removing the line check from the test on line 31...

Thanks!

@kinke
Copy link
Member

kinke commented Mar 3, 2022

Hmm, I don't recall seeing such a mixture of available/missing line infos for unoptimized code OTOH, but yeah, ignoring the line sounds good enough.

@kinke
Copy link
Member

kinke commented Mar 3, 2022

LGTM after a superficial glance without checking the actual logic/tests. Please merge yourself if happy. [Next time, please use version (LDC) with the space; I usually grep for that string and would prefer not having to use a regex. ;)]

@kinke
Copy link
Member

kinke commented Mar 3, 2022

[If you want to add a release note entry, please extend the new master section of CHANGELOG.md.]

… it, which changes the ABI (adds data structure fields) for LDC.

Actual execution of code to support sanitizers (e.g. keeping track of ASan data and scanning the ASan FakeStack) is disabled by default and requires building
Some CI systems have only partial line data (reason unknown).
@JohanEngelen JohanEngelen enabled auto-merge (squash) March 3, 2022 22:53
@JohanEngelen JohanEngelen merged commit eec920e into ldc-developers:master Mar 4, 2022
@JohanEngelen JohanEngelen deleted the fakestack branch March 4, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants