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] LLVM 17 does not detect ODR violations previously found using ASAN #67677

Closed
justincady opened this issue Sep 28, 2023 · 5 comments
Closed

Comments

@justincady
Copy link
Contributor

Using ASAN with LLVM 17 fails to detect ODR violations that are detected with LLVM 16 and earlier.

I bisected to this commit: a8d3ae7

I'm not sure if this is intended behavior (which would surprise me as the symbol is used) or an unintended consequence. Here's a reproducer:

// shared.c
int in_shared(void) { return 1; }
int (*fptr)(void) = in_shared;
// static.c
int in_static(void) { return 2; }
int (*fptr)(void) = in_static;
// main.c
extern int in_shared(void);
extern int in_static(void);
extern int (*fptr)(void);
int main() { return fptr() + in_static() + in_shared(); }
#!/bin/bash

CLANG=/path/to/clang
$CLANG --version | head -1 | cut -d " " -f1-3
$CLANG -fsanitize=address -fPIC -shared shared.c -o shared.so
$CLANG -fsanitize=address -c static.c
ar rs static.a static.o
$CLANG -fuse-ld=lld -fsanitize=address -Wl,-rpath,'$ORIGIN' shared.so static.a main.c
./a.out

Running it with LLVM 16:

$ ./build.sh
clang version 16.0.6
=================================================================
==28316==ERROR: AddressSanitizer: odr-violation (0x55e177c12b40):
  [1] size=8 'fptr' static.c
  [2] size=8 'fptr' shared.c
These globals were registered at these points:
  [1]:
    #0 0x55e177b4b217 in __asan_register_globals /[...]/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x55e177c0d33b in asan.module_ctor static.c

  [2]:
    #0 0x55e177b4b217 in __asan_register_globals /[...]/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7fcfa36daf6b in asan.module_ctor shared.c

==28316==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'fptr' at static.c
==28316==ABORTING
$

Running it with LLVM 17:

$ ./build.sh
clang version 17.0.1
$

Flipping the default back by applying -fno-sanitize-address-globals-dead-stripping brings back the error with LLVM 17:

$ ./build.sh
clang version 17.0.1
=================================================================
==28382==ERROR: AddressSanitizer: odr-violation (0x557d7e463e20):
  [1] size=8 'fptr' static.c
  [2] size=8 'fptr' shared.c
[...]

But the default behavior appears to hide ODR violations, which seems like a bug to me.

cc @MaskRay

@MaskRay MaskRay self-assigned this Sep 28, 2023
@Endilll Endilll added compiler-rt:asan Address sanitizer regression and removed new issue labels Sep 29, 2023
MaskRay added a commit to MaskRay/llvm-project that referenced this issue Sep 29, 2023
…dule_ctor (llvm#67745)

On ELF platforms, when there is no global variable, COMDAT asan.module_ctor is
created with no `__asan_register_elf_globals` calls. If this COMDAT is the
prevailing copy selected by the linker, the linkage unit will have no
`__asan_register_elf_globals` call: the redzone will not be poisoned and ODR
violation checker will not work (llvm#67677).

This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call `__asan_register_globals`
(`InstrumentGlobalsWithMetadataArray`) do not use COMDAT.

To fix llvm#67677:

* Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
* Call `__asan_register_elf_globals` even if there is no global variable.

Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call `__asan_register_elf_globals`. However, the asan.module_ctor
function cannot be eliminated by the linker.

Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.
```
echo > a.cc  # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc  # with global variable, with uniqueModuleId
echo 'int g;' > c.cc  # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
  for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
    for f in a.cc b.cc c.cc; do
      echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
      /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
    done
  done
done
```
@MaskRay
Copy link
Member

MaskRay commented Sep 29, 2023

/cherry-pick 1a4b9b6

Would be nice to have this in 17.0.2

@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

/branch llvm/llvm-project-release-prs/issue67677

@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

/pull-request llvm/llvm-project-release-prs#714

@MaskRay
Copy link
Member

MaskRay commented Sep 29, 2023

1a4b9b6 had an issue and was reverted with a reland.

/cherry-pick 16eed8c

@MaskRay MaskRay reopened this Sep 29, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

/branch llvm/llvm-project-release-prs/issue67677

@tru tru moved this from Needs Triage to Needs Review in LLVM Release Status Oct 2, 2023
@tru tru moved this from Needs Review to Done in LLVM Release Status Oct 17, 2023
tru pushed a commit that referenced this issue Oct 17, 2023
…dule_ctor (#67745)

On ELF platforms, when there is no global variable and the unique module ID is
non-empty, COMDAT asan.module_ctor is created with no
`__asan_register_elf_globals` calls. If this COMDAT is the prevailing copy
selected by the linker, the linkage unit will have no
`__asan_register_elf_globals` call: the redzone will not be poisoned and ODR
violation checker will not work (#67677).

This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call `__asan_register_globals`
(`InstrumentGlobalsWithMetadataArray`) do not use COMDAT.

To fix #67677:

* Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
* Call `__asan_register_elf_globals` even if there is no global variable.
* If the unique module ID is empty, don't call SetComdatForGlobalMetadata:
  placing `@.str` in a COMDAT would incorrectly discard internal COMDAT `@.str`
  in other compile units.

Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call `__asan_register_elf_globals`. However, the asan.module_ctor
function cannot be eliminated by the linker.

Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.
```
echo > a.cc  # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc  # with global variable, with uniqueModuleId
echo 'int g;' > c.cc  # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
  for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
    for f in a.cc b.cc c.cc; do
      echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
      /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
    done
  done
done
```

---

Identical to commit 16eed8c.
6420d33 is an incorrect revert for genuine
purely internal issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment