__asan_globals_registered is not comdat when building a staticlib with LTO

Disclaimer: I tried to create a testcase from scratch, but for some reason I wasn't able to find a way to trigger the use of __asan_register_elf_globals instead of __asan_globals_register.

STR:

Clone https://github.com/glandium/nss-builtins
cd nss-builtins
RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release
objdump -t target/release/libbuiltins_static.a | grep asan_globals_registered

Actual output:

0000000000000000 l     O .bss.___asan_globals_registered	0000000000000008 ___asan_globals_registered
0000000000000000 l    d  .bss.___asan_globals_registered	0000000000000000 .bss.___asan_globals_registered

Expected output:
Something like:

0000000000000008       O *COM*	0000000000000008 .hidden ___asan_globals_registered

This doesn't happen without LTO.
The unfortunate consequence is that when the resulting static library is linked with C or C++ code compiled with clang with -fsanitize=address -fsanitize-address-globals-dead-stripping (that latter flag is now default in clang trunk), which also uses __asan_register_elf_globals/__asan_globals_registered, ODR violation detection kick in complaining about globals defined multiple times, because both the clang-side asan constructor and the rust asan constructor register all the globals. Normally, what happens is that they both use the same __asan_globals_registered (thus it normally being *COM*), and set its value, so that only one constructor registers the globals. With the LTOed staticlib, what happens is that there are two distinct __asan_globals_registered, so both constructors go through.

rustc +nightly --version --verbose:

rustc 1.72.0-nightly (d9c13cd45 2023-07-05)
binary: rustc
commit-hash: d9c13cd4531649c2028a8384cb4d4e54f985380e
commit-date: 2023-07-05
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

(Edit: fixed typos, changed the peculiar setup with -Clto and -Cembed-bitcode=yes to the more normal LTO, which shows the problem too)

https://bugs.chromium.org/p/chromium/issues/detail?id=1459233 is tracking this for Chromium as it causes our asan bots to fail.

Here is the code which creates the __asan_globals_registered global:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L2216

It's interesting that it uses common linkage (*COM* as you said). Common linkage should work, but to me it is a surprising choice. I would've expected this global to use linkonce_odr linkage and be marked comdat, which is what you would get for a C++17 inline global.

I don't know how Rust is producing LTOed static libraries, but maybe somewhere along the way there is a bug in how LTO is handling common linkage globals.

One possible summary of this issue is that the ODR violation detector is suffering from an ODR violation, we have two flags when we should have one.

One possible summary of this issue is that the ODR violation detector is suffering from an ODR violation, we have two flags when we should have one.

I like this take :)

I consider myself quite familiar with asan, LTO, Clang Driver, but I know very little about Rust.

rm -r target/release/libbuiltins_static.a
RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release --verbose

gives me this command (after removing --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat):

rustc --crate-name builtins_static --edition=2021 src/lib.rs --diagnostic-width=159 --crate-type staticlib --emit=dep-info,link -C opt-level=3 -C lto -C metadata=9ef9aaa79a14308e -C extra-filename=-9ef9aaa79a14308e --out-dir /tmp/p/nss-builtins/target/release/deps -L dependency=/tmp/p/nss-builtins/target/release/deps --extern pkcs11_bindings=/tmp/p/nss-builtins/target/release/deps/libpkcs11_bindings-cb6ed583361f31fa.rlib --extern smallvec=/tmp/p/nss-builtins/target/release/deps/libsmallvec-e6a97ae5d501d626.rlib -Zsanitizer=address

% ar t /tmp/p/nss-builtins/target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a | wc -l
177
% ar x /tmp/p/nss-builtins/target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o
% readelf -Ws builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o | grep ___asan_globals_registered
   117: 0000000000000000     8 OBJECT  LOCAL  DEFAULT 3732 ___asan_globals_registered
  2944: 0000000000000000     0 SECTION LOCAL  DEFAULT 3732 .bss.___asan_globals_registered
% llvm-nm -gU builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o
0000000000000000 T BUILTINSC_GetFunctionList
0000000000000000 V DW.ref.rust_eh_personality
0000000000000000 D _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17h244e25de9c1d3c88E
0000000000000000 T rust_eh_personality

Most defined symbols are localized. I do not know why the 4 symbols are special and rustc lto doesn't localize them.

I don't know how to rerun the rustc with a locally built (./x.py build with config.toml containing [rust]\ndebug=true).
I suspect that preventing ___asan_globals_registered from being localized will fix this bug.

-fcommon is consider bad nowadays but the ___asan_globals_registered COMMON symbol use case is fine. It is like a COMDAT group containing just a variable. COMMON is more size efficient than using a COMDAT group (there is a size overhead due to a 64 byte Elf64_Shdr header).

If we use a COMDAT for ___asan_globals_registered but rustc compiler/rustc_codegen_llvm/src/back/lto.rs still localizes the symbol, then we'd still have this bug.

[Fuchsia] Add ASAN_OPTIONS in asan build

@MaskRay

Let me try to help move this bug forward by attempting to answer some of the questions above. I have limited experience with linkers, asan, and compilers, so please shout if there are any mistakes below.

RE: @MaskRay: I don't know how to rerun the rustc with a locally built (./x.py build)

I tweaked the original cargo cmdline (from the first comment/report on this bug) by adding RUSTC=<path to locally built rustc> and adding -Zbuild-std --target x86_64-unknown-linux-gnu (and other than that I've rerun the original repro steps + debugging steps from your earlier comment at #113404 (comment)):

RUSTC=$HOME/src/github/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release -Zbuild-std --target x86_64-unknown-linux-gnu

(Note that this changes the path where the build artifacts are - e.g. target/x86_64-unknown-linux-gnu/release/deps/libbuiltins_static-9bce9c00959dd948.a instead of target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a)

RE: @MaskRay: I do not know why the 4 symbols are special and rustc lto doesn't localize them.

I am not sure if these are the reasons, but this is what I've found for some of the symbols emitted by llvm-nm -gU ...:

I see that rust/compiler/rustc_codegen_llvm/src/context.rs marks the EH personality with llvm::UnnamedAddr::Global
AFAIU _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17h244e25de9c1d3c88E corresponds to the static initializer in rust/library/std/src/sys/unix/args.rs which is marked as #[used]
I see that BUILTINSC_GetFunctionList comes from the repro and is declared as #[no_mangle] which AFAIU tells rustc to export the function.
- FWIW, I also see that rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs special-cases "rust_eh_personality" (see here), but changing this to cover names with "asan" substring didn't seem to have an effect on the llvm-nm -gU ... output)

RE: @MaskRay: I suspect that preventing ___asan_globals_registered from being localized will fix this bug.

Can you please elaborate on that? From your #113404 (comment), it seems that you are saying that a change in rustc is needed - did I get that right?

OTOH, it seems that ___asan_globals_registered comes from outside of rustc sources - it comes from llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp and therefore it seems that maybe we need to change how it is marked for linking by LLVM? Interestingly the comment there mentions "a local symbol" - not sure if this is relevant:

// ASan version script has __asan_* wildcard. Triple underscore prevents a
// linker (gold) warning about attempting to export a local symbol.
const char kAsanGlobalsRegisteredFlagName[] = "___asan_globals_registered";

@eugenis

/cc @eugenis who AFAICT added the kAsanGlobalsRegisteredFlagName comment above in llvm/llvm-project@964f466#diff-74cc02fff013e3ab08aefcad70e555b1177549f8403d03395c856c3f8c5b5875

@rnk

Hmmm... I now think that it still makes sense to review and attempt to land the rustc PR at https://github.com/rust-lang/rust/pull/114946I even if we (eventually) preserve these ASAN symbols via a separate LLVM PR:

At least part of the rustc PR at Preserve ASAN-related symbols during LTO. #114946 is desirable in the long-term (i.e. even once LLVM changes are made). Specifically, we want to land the changes that make symbol_export.rs unaware of __llvm_profile_raw_version, __llvm_profile_filename, __msan_keep_going, __msan_track_origins names (i.e. keep the removal of code in 2c75640#diff-6d46444b86c506127f8dd251a0c949845de78baec01de25b7524b3b10d90efd7).
It seems desirable for rustc to work correctly even if built with older LLVM versions
We can consider opening a follow-up issue against the llvm-project (and maybe adding a TODO in the rustc PR?)

RE: @rnk: #113404 (comment): Is it possible to emulate what Rust is doing using opt -internalize?

Maybe. I don't know how to check. Sorry.

FWIW, I see that LLVM-level tests that use opt -internalize take LLVM-IR as input. This probably means that ___asan_globals_registered would have to be hardcoded/simulated in the test input (rather than generated by the real ASAN). Maybe this is ok.

RE: @rnk: #113404 (comment): Putting this list of symbols in LLVM sounds practical to me.

The llvm-project/llvm/lib/Transforms/IPO/Internalize.cpp source file has quite elaborate code for preserving some symbols. I wonder if ASAN / MSAN / etc symbols should be protected by any of the existing mechanisms:

Would we consider adding ___asan_globals_registered, __msan_track_origins, etc. into InteranlizePass::AlwaysPreserved (e.g. somewhere here)
Or maybe the already-existing preservation of Used variables should kick-in (but for some reason doesn't work for ASAN/MSAN/etc)?
Or maybe the already existing checks in InternalizePass::shouldPreserveGV should kick-in (but for some readon don't work for ASAN/MSAN/etc). Or maybe these checks need to be extended in a generic way (rather than teaching Internalize.cpp about __asan and/or __msan prefixes).

@glandium

Status update / summary (I edited this comment on 2023-08-25 at 10:47 PST to add one other potential next step at the very end of the comment):

Impact of the bug (sorry for only listing the impact on Chromium, but this is what I am personally aware of):
- Blocks further Chromium adoption of Rusty QR Code Generation which is enabled on all platforms except ChromeOS (where this bug is the only known blocker).
- Triggered "temporary" disabling of ODR violation detection on Fuchsia (see https://crbug.com/1459233#c19)
Reproability / testability status:
- We know how to detect the problem caused by this bug:
  - At LLVM-IR level (@___asan_globals_registered = common hidden global vs ... = internal global). This translates nicely into a new tests/codegen/sanitizer/address-sanitizer-globals-tracking.rs test.
  - At nm level (see __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment)). This was checked manually before and after the fix in __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment)
- We don't have a minimized end-to-end repro (e.g. rustc invocations + maybe clang invocations => incorrect ASAN report about an ODR violation)
  - In https://crbug.com/1467360#c4 I am tentatively suggesting that loading dynamic libraries/plugins might be a required component of the repro
  - @glandium has trouble in __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment) with creating a testcase from scratch
  - I think that both __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment) and https://crbug.com/1467360 suggest that mixing C++ and Rust is a required component of the repro
We considered two fix options:
- Option 1 (rejected for now; see PR at Mark ___asan_globals_registered as an exported symbol for LTO #114642): Tweaking rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs
  - This mimics 4053e25, d8c661a, and 2c0845c
  - This doesn't work as well here, because it requires replicating LLVM's checks to only expect __asan_globals_registered for ELF and MachO targets (see __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment), __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment), and __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment))
- Option 2 (currently being pursued; see PR at Preserve ASAN-related symbols during LTO. #114946): Removing ASAN/MSAN/PGO/LTO-related code from rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs and instead controlling LTO internalization in compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp via PreserveFunctions lambda (using symbol name prefixes like __msan instead of specific names like __msan_track_origins).
In the code review @nikic requested adding ThinLTO tests
- In a Zulip discussion we concluded that ThinLTO cannot be tested via tests/codegen
- If the repro indeed requires mixing C++ and Rust toolchains (see the bullet above about a minimized end-to-end repro) then it seems that tests/ui (run-pass, check-run-results) cannot provide test coverage (AFAIU tests/ui can only invoke `rustc).
- It may be possible to invoke rustc and clang via tests/run-make, but this requires actually having an end-to-end repro. I spent some time trying to create a repro yesterday but failed.
- I note that the previous fixes (for MSAN) were not required to add ThinLTO-related test coverage. The new test at tests/codegen/sanitizer/address-sanitizer-globals-tracking.rs mimics and covers similar scenarios as (already existing) tests/codegen/sanitizer/sanitizer-recover.rs and tests/codegen/sanitizer/memory-track-origins.rs
- I note that compiler/rustc_codegen_llvm/src/back/lto.rs does indeed call LLVMRustRunRestrictionPass only from fat_lto. In other words, Option 2 stops special-handling __msan_track_origins, __msan_keep_going, __llvm_profile_raw_version, and __llvm_profile_filename during ThinLTO. OTOH, the problem with these symbols was internalization which happens during fat LTO, but I am not sure if happens during ThinLTO (definitely not the cross-module kind).

Can we discuss the next steps?:

Can we discuss whether ThinLTO tests are a hard requirement to land the fix? Maybe we can land the fix at Preserve ASAN-related symbols during LTO. #114946 in its current shape (with a LLVM-IR-level regression test covering ASAN + no LTO, and ASAN + fat LTO).
If ThinLTO or end-to-end tests are required, then can somebody please help with creating a minimal repro? It seems that either tests/run-make/cross-lang-lto-clang or tests/run-make/cross-lang-lto-pgo-smoketest might be good role models. (Let me ask in https://crbug.com/1459233#c21 if anyone else from Chromium might be able to help).
Maybe we should also look if the fix can be made in LLVM instead (as suggested in __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment)). It seems that the next step (pointed out by @rnk in __asan_globals_registered is not comdat when building a staticlib with LTO #113404 (comment)) would be figuring out if that bug at hand can be reproted/simulated using opt -internalize (whatever that means in the world of LLVM tests :-)).

@bjorn3

@bjorn3, in #114946 (comment) you've suggested that there might be issue with Option 2 of the fix. Let me partially reply here, because I have some questions that are not related to Option 2.

Do you think we should proceed with Option 1 (or is there another approach that you'd suggest)? If so, then I assume that you agree that to avoid the x86_64-apple-1 test failures we should only inject __asan_globals_registered in exported_symbols_provider_local when the symbol is actually present (i.e. when ModuleAddressSanitizer::InstrumentGlobalsELF and/or ModuleAddressSanitizer::InstrumentGlobalsMachO inject the symbol). Could you please help me understand how to tweak #114642 to do this? How can exported_symbols_provider_local detect ELF and/or MachO targets? Do you think it would be okay to copy-and-paste let is_like_elf = ... from codegen_attrs.rs? What would you suggest for detecting MachO targets? (I don't see code for detecting MachO targets outside of compiler/rustc_codegen_cranelift/src/lib.rs which I assume can't be used outside of cranelist.)

Do you think we should proceed with Option 1

I think so, but I may be misunderstanding what exactly address sanitizer needs in terms of linkage.

How can exported_symbols_provider_local detect ELF and/or MachO targets?

Like this:

rust/compiler/rustc_codegen_ssa/src/back/metadata.rs

Lines 217 to 225 in 22d41ae

    
           let binary_format = if sess.target.is_like_osx { 
        
               BinaryFormat::MachO 
        
           } else if sess.target.is_like_windows { 
        
               BinaryFormat::Coff 
        
           } else if sess.target.is_like_aix { 
        
               BinaryFormat::Xcoff 
        
           } else { 
        
               BinaryFormat::Elf 
        
           };

Maybe extracting this into a function to ensure it is kept in sync makes sense.

Mark `___asan_globals_registered` as an exported symbol for LTO

How can exported_symbols_provider_local detect ELF and/or MachO targets?

Like this:

...

Maybe extracting this into a function to ensure it is kept in sync makes sense.

Thanks! That helps :-). Not sure how I managed to miss this piece of code when greepping the source code for BinaryFormat... :-/

FWIW I've applied the changes suggested above to the newly pushed version of #114642 (e.g. see d9abc46#diff-aa810a3be0834da891b171a8b04b09d0d3bbb76ac27c757cd232235099e062d2)

Preserve `___asan_globals_registered` symbol during LTO.

Auto merge of rust-lang#114946 - anforowicz:generic-fix-for-asan-lto,…

Auto merge of #114946 - anforowicz:generic-fix-for-asan-lto, r=tmiasko

Navigation Menu

Repository navigation

__asan_globals_registered is not comdat when building a staticlib with LTO #113404

31 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	let binary_format = if sess.target.is_like_osx {
	BinaryFormat::MachO
	} else if sess.target.is_like_windows {
	BinaryFormat::Coff
	} else if sess.target.is_like_aix {
	BinaryFormat::Xcoff
	} else {
	BinaryFormat::Elf
	};

__asan_globals_registered is not comdat when building a staticlib with LTO #113404

Description

Activity

danakj commented on Jul 12, 2023

rnk commented on Jul 12, 2023

glandium commented on Jul 12, 2023

MaskRay commented on Jul 12, 2023

MaskRay commented on Jul 12, 2023

anforowicz commented on Jul 27, 2023

anforowicz commented on Jul 27, 2023

31 remaining items

anforowicz commented on Aug 17, 2023

anforowicz commented on Aug 25, 2023

anforowicz commented on Aug 25, 2023

bjorn3 commented on Aug 26, 2023

anforowicz commented on Aug 28, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions