ASAN Instrumentation removed when using optimizations

[llvmbot]

Bugzilla Link	52382
Resolution	FIXED
Resolved on	Nov 10, 2021 20:48
Version	trunk
OS	Linux
Blocks	#51489
Reporter	LLVM Bugzilla Contributor
CC	@aeubanks,@PiJoules,@tstellar,@vitalybuka

Extended Description

The following code does not emit clang instrumentation when compiling with optimizations:

#include <cstdlib>

int global_array[100] = {-1};

int main(int argc, char **argv) {
  return global_array[std::atoi("3") + 100];  // global buffer overflow
}

Code generated with -fsanitize=address -O -g

main:                                   # @​main
        mov     eax, dword ptr [rip + global_array+412]
        ret

Shouldn't the code have asan instrumentation? When compiling with -O0 the instrumentation is there, as well as compiling with -flegacy-pass-manager

[PiJoules]

Ok, I think I found the underlying issue. So asan (under the new PM) isn't instrumenting the load in main because it considers it incorrectly considers it a "safe access".

For my explanation, I've been compiling:

int global_array[100] = {-1};
int main(void) { return global_array[103]; }

with bin/clang++ /tmp/test.c -fsanitize=address [-fno-experimental-new-pass-manager].

So the hood, it looks like asan extends the size of global_array from 400 bytes to 512. The added 112 bytes is the redzone asan adds around globals. The issue is that asan, when checking the global load in main, checks the load against this extended global rather than the initial global. The load is 412 bytes from the start of the array, but the extended array is 512 bytes long, so arithmetic-wise, accessing that array is valid.

Now asan (for both legacy and new PM) is composed of 2 passes: a function pass that loops over functions searching for interesting accesses, and a module pass that instruments globals. The module pass is in charge of changing globals to allocate for the redzone. We only see this error for the new PM because the module pass is run BEFORE the function pass:

// Output when using -Xclang -fdebug-pass-manager
Running pass: RequireAnalysisPass<llvm::ASanGlobalsMetadataAnalysis, llvm::Module> on [module]
Running analysis: ASanGlobalsMetadataAnalysis on [module]
Running pass: ModuleAddressSanitizerPass on [module]
...
Running pass: AddressSanitizerPass on main

Under the legacy PM, the module pass runs AFTER the function pass:

Profile summary info
  ModulePass Manager
...
    ASanGlobalsMetadataWrapperPass
    FunctionPass Manager
      AddressSanitizerFunctionPass
    ASanGlobalsMetadataWrapperPass
    ModuleAddressSanitizer
...

So the function pass in the legacy PM operates on the global before it gets instrumented. It looks like the right solution might be somehow ensuring the module pass runs after the function pass in the new PM. I think this is doable in the legacy PM but I'm not entirely sure how this is done in the new PM. CC'ing Arthur who has done a lot of new PM work and may know the correct solution.

[aeubanks]

Should be a fairly easy fix in BackendUtil.cpp by swapping the order we add the function/module pass.

llvm-project/clang/lib/CodeGen/BackendUtil.cpp

Line 1188 in 3de3ca3

MPM.addPass(ModuleAddressSanitizerPass(CompileKernel, Recover,

[PiJoules]

Looks like https://reviews.llvm.org/D112732 already fixed this by removing the function pass entirely and just keeping the module pass, but ensuring function instrumentation happens before module instrumentation within that pass.

I have https://reviews.llvm.org/D113143 up which adds a regression test.

[vitalybuka]

This is broken in clang 13.
Do we want cherry-picks?

[PiJoules]

Oh yeah, we'll want to cherry pick this. CCing tstellar@. Would you be able to cherry pick a55c4ec which contains this fix?

[vitalybuka]

I afraid simple cherry-pick is not possible.

This is the minimal relevant patch https://reviews.llvm.org/D113529
And the we can cherry-pick 16130d00b0cb3fd4e6b0f824fbc3787ad551e797 test.

[tstellar]

I afraid simple cherry-pick is not possible.

This is the minimal relevant patch https://reviews.llvm.org/D113529
And the we can cherry-pick 16130d00b0cb3fd4e6b0f824fbc3787ad551e797 test.

I don't see this commit in tree, is there a typo?

[vitalybuka]

I afraid simple cherry-pick is not possible.

This is the minimal relevant patch https://reviews.llvm.org/D113529
And the we can cherry-pick 16130d00b0cb3fd4e6b0f824fbc3787ad551e797 test.

I don't see this commit in tree, is there a typo?
Sorry, it should be:

https://reviews.llvm.org/D113529
and
a55c4ec

[vitalybuka]

https://reviews.llvm.org/D113529
and
a55c4ec

Typo again :(

https://reviews.llvm.org/D113529
and
456a7e5