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

Asan with Windows EH generates __asan_xxx runtime calls without required funclet tokens #64990

Closed
sylvain-audi opened this issue Aug 25, 2023 · 3 comments · Fixed by #82533
Closed
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior compiler-rt:asan Address sanitizer llvm:transforms platform:windows

Comments

@sylvain-audi
Copy link
Contributor

This issue has been discussed in #39667 as well as google/sanitizers#749

As mentionned by @rnk in a comment: "ASan instrumentation isn't inserting calls into EH pads with the right funclet token bundle, so any attempts to do ASan checks inside funclets will be removed by WinEHPrepare. "

It seems that not only WinEHPrepare will remove the calls themselves but it also discards the entire Basic Block around it, replacing the content with unreachable statement.

Here is a repro:

char buff1[6] = "hello";
char buff2[6] = "hello";

int main(int argc, char **argv) {
  int result = 1;
  try {
    throw 1;
  } catch (...) {
    // make asan generate call to __asan_memcpy that should report an error due to memory overflow.
    __builtin_memcpy(buff1, buff2 + 3, 6);
    result = 0;
  }
  return result;
}

See in Compiler Explorer: https://godbolt.org/z/Tacdhsz4T
Notice, in the LLVM Opt Pipeline Viewer, the WinEHPrepare pass of main: the entire catch Basic block and successors get transformed into a single unreachable statement.

@sylvain-audi sylvain-audi added bug Indicates an unexpected problem or unintended behavior compiler-rt:asan Address sanitizer platform:windows labels Aug 25, 2023
@sylvain-audi sylvain-audi self-assigned this Aug 25, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 25, 2023

@llvm/issue-subscribers-bug

@rnk
Copy link
Collaborator

rnk commented Aug 28, 2023

This is true, this was the intended behavior. The thinking was:

  • calls to functions with the wrong funclet are UB
  • we can transform them to unreachable, and then run the standard unreachable block cleanup pass
  • if we do this, it will enforce the invariant that all blocks have a unique funclet color without having to clone blocks

It's the kind of error recovery that only a compiler engineer would think is reasonable ("What do you do when something impossible happens, abort and report an error? No, just delete the code, the impossible can't happen!"), but that's what we did, I can't really defend it too strongly.

We could certainly revise the decision. The smallest most incremental change would be to replace the call with a trap followed by unreachable, which won't propagate backwards.

@sylvain-audi
Copy link
Contributor Author

Thanks for the explanation!

I intended the scope of this issue to be limited to the bug where asan doesn't add a required funclet opbundle, causing UB. A patch is currently in review for this, which simply adds the missing opbundle.

I wanted to isolate this bug as a new issue, as I realized that the patch I mentioned doesn't entirely fix the issues where it was described in the comments (#39667 and google/sanitizers#749). This way I could close this issue after the patch lands.

sylvain-audi added a commit to sylvain-audi/llvm-project that referenced this issue Feb 21, 2024
…ality

Previously, when ASan instrumentation introduced runtime calls into EH pads, the appropriate funclet token expected by WinEHPrepare were missing.
The BB is then seen as invalid and discard by WinEHPrepare, leading to invalid code that crashes.

Also fixed localescape test, switching its EH personality to match code without funclets.

This PR is based on the Phabricator patch https://reviews.llvm.org/D143108

Fixes llvm#64990
sylvain-audi added a commit that referenced this issue Mar 1, 2024
This is in preparation for PR
#82533.

Here we Introduce a test for asan instrumentation where invalid code is
output (see bug #64990)
The `CHECK` lines are generated using `update_test_checks.py` script.
The actual fix PR will udpate this test to highlight the changes in the
generated code.
sylvain-audi added a commit to sylvain-audi/llvm-project that referenced this issue Mar 1, 2024
…ality

Previously, when ASan instrumentation introduced runtime calls into EH pads, the appropriate funclet token expected by WinEHPrepare were missing.
The BB is then seen as invalid and discard by WinEHPrepare, leading to invalid code that crashes.

Also fixed localescape test, switching its EH personality to match code without funclets.

This PR is based on the Phabricator patch https://reviews.llvm.org/D143108

Fixes llvm#64990
sylvain-audi added a commit to sylvain-audi/llvm-project that referenced this issue Mar 7, 2024
…ality

Previously, when ASan instrumentation introduced runtime calls into EH pads, the appropriate funclet token expected by WinEHPrepare were missing.
The BB is then seen as invalid and discard by WinEHPrepare, leading to invalid code that crashes.

Also fixed localescape test, switching its EH personality to match code without funclets.

This PR is based on the Phabricator patch https://reviews.llvm.org/D143108

Fixes llvm#64990
sylvain-audi added a commit to sylvain-audi/llvm-project that referenced this issue Mar 8, 2024
…ality

Previously, when ASan instrumentation introduced runtime calls into EH pads, the appropriate funclet token expected by WinEHPrepare were missing.
The BB is then seen as invalid and discard by WinEHPrepare, leading to invalid code that crashes.

Also fixed localescape test, switching its EH personality to match code without funclets.

This PR is based on the Phabricator patch https://reviews.llvm.org/D143108

Fixes llvm#64990
sylvain-audi added a commit that referenced this issue Mar 8, 2024
…by EH personality (#82533)

Previously, runtime calls introduced by ASan instrumentation into EH
pads were missing the funclet token expected by WinEHPrepare.
WinEHPrepare would then identify the containing BB as invalid and
discard it, causing invalid code generation that most likely crashes.

Also fixed localescape test, switching its EH personality to match code
without funclets.

This PR is based on the Phabricator patch
https://reviews.llvm.org/D143108

Fixes #64990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler-rt:asan Address sanitizer llvm:transforms platform:windows
Projects
None yet
4 participants