DLL load order of ASan causes false reports on `realloc` calls during DLL initialization

[alvinhochun]

Short version: If the program tries to realloc a block of memory that had been malloc-ed before the ASan runtime was initialized, ASan reports an error for "attempting free on address which was not malloc()-ed". Making ASan load earlier can fix this.

Long version:

When building an EXE with ASan enabled, even if it depends on other user library DLLs which have not been rebuilt with ASan, it should still work fine because ASan intercepts allocation functions to capture allocations across all DLLs. However, ASan misses any allocations made before libclang_rt.asan_dynamic-x86_64.dll is loaded.

https://reviews.llvm.org/D25946 implemented a change to ignore such allocations made by ucrt when deallocating. In practice, this also ignores any allocations made by other user DLLs. This generally includes C++ dynamic initialization (e.g. global static variables). (This slightly reduces ASan coverage, but at least the program still runs and ASan is able to instrument the rest of the program, which to me is acceptable.) However, this only happens for Deallocate; Reallocate does not contain the same checks. If an uninstrumented DLL allocates a pointer during initialization and this pointer is later reallocated, ASan will report "attempting free on address which was not malloc()-ed".

Reproducer

asan_load_test.zip

I tested this with llvm-mingw-20230320-ucrt-x86_64 from https://github.com/mstorsjo/llvm-mingw/releases/tag/20230320 .

To build it, change Makefile to point CXX to clang++, then run mingw32-make to build. Set ASAN_OPTIONS=verbosity=1 (optional) and run main.exe to test.

Proposed Fix 1: Reordering link flags from the Clang driver**

I realized that it is possible to influence the Windows library loader to load the ASan DLL earlier than most other user DLLs, by making it the first entry in the Import Directory Table, earlier than other DLLs. When linking with LLD, the order in which the import libraries are specified affects how the Import Directory Table is arranged. This can be verified with the reproducer with these steps:

1. Run the command to build main.exe manually with --verbose
2. Run the printed build command manually
3. Edit the link command (ld.lld) to move libclang_rt.asan_dynamic-x86_64.dll.a before other input files, then run it manually
4. Run llvm-readobj --coff-imports main.exe to verify that the ASan dll is listed first
5. Run main.exe should no longer trigger an ASan report.

For the MinGW Clang driver, these link flags are added in:

llvm-project/clang/lib/Driver/ToolChains/MinGW.cpp

Lines 295 to 311 in 8b51340

	if (Sanitize.needsAsanRt()) {
	// MinGW always links against a shared MSVCRT.
	CmdArgs.push_back(TC.getCompilerRTArgString(Args, "asan_dynamic",
	ToolChain::FT_Shared));
	CmdArgs.push_back(
	TC.getCompilerRTArgString(Args, "asan_dynamic_runtime_thunk"));
	CmdArgs.push_back("--require-defined");
	CmdArgs.push_back(TC.getArch() == llvm::Triple::x86
	? "___asan_seh_interceptor"
	: "__asan_seh_interceptor");
	// Make sure the linker consider all object files from the dynamic
	// runtime thunk.
	CmdArgs.push_back("--whole-archive");
	CmdArgs.push_back(
	TC.getCompilerRTArgString(Args, "asan_dynamic_runtime_thunk"));
	CmdArgs.push_back("--no-whole-archive");
	}

I presume the solution is as simple as moving this block of code further up. (MSVC might also need the same fix, but I haven't checked.)

Downside: This only works when linking through Clang. If LLD is invoked directly, the user needs to be aware of the ordering of the ASan link flags.

Proposed Fix 2: Teach LLD to always put the ASan DLL in first**

This fixes the issue for both linking through Clang or by the user invoking LLD directly. However, this introduces a special case so I don't really think this is a good idea.

Proposed Fix 3: Teach ASan Reallocate to not report on uninstrumented allocations**

We can probably apply the same thing in https://reviews.llvm.org/D25946 to Reallocate and not bother with trying to change the DLL load order. However, this reduces ASan coverage so I think this should be avoided if possible.

---

cc @mstorsjo

[llvmbot]

@llvm/issue-subscribers-clang-driver

[llvmbot]

@llvm/issue-subscribers-bug

[mstorsjo]

Thanks for digging up the root causes for this issue!

I agree that 2. probably isn’t a good idea. 1. probably is good to do unless there are other technical reasons for having the ordering we have right now. Manually calling lld isn’t generally something that users do for mingw build cases, just like with unix linkers, since there’s so many boilerplate options one needs to set to get anything working. With msvc/clang-cl use of asan, the library gets brought in through a different route though (embeds coff directives, iirc?) - does that setup have the same issue, or does asan already get linked first there?

I think option 3. probably would be good to do anyway, for consistency. Yes it reduces test coverage slightly, but reallocation is less common than plain deallocation anyway, so I don’t think it’s a big deal, if we’ve already made that tradeoff for deallocation.

[alvinhochun]

Thanks for digging up the root causes for this issue!

I agree that 2. probably isn’t a good idea. 1. probably is good to do unless there are other technical reasons for having the ordering we have right now. Manually calling lld isn’t generally something that users do for mingw build cases, just like with unix linkers, since there’s so many boilerplate options one needs to set to get anything working. With msvc/clang-cl use of asan, the library gets brought in through a different route though (embeds coff directives, iirc?) - does that setup have the same issue, or does asan already get linked first there?

I just tried it a bit with clang-cl from 16, when compiling with /MD (multithread DLL run-time), it looks like the ASan link flags are already added before other imports. I tried with passing the .lib files directly as input files as well as passing them after /link, both methods work. The test program does not produce the false report either, so it jjclang-cl is already working fine in this regard.

The flags are added from:

llvm-project/clang/lib/Driver/ToolChains/MSVC.cpp

Lines 196 to 225 in 0be1fba

	if (TC.getSanitizerArgs(Args).needsAsanRt()) {
	CmdArgs.push_back(Args.MakeArgString("-debug"));
	CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
	if (TC.getSanitizerArgs(Args).needsSharedRt() ||
	Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd)) {
	for (const auto &Lib : {"asan_dynamic", "asan_dynamic_runtime_thunk"})
	CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
	// Make sure the dynamic runtime thunk is not optimized out at link time
	// to ensure proper SEH handling.
	CmdArgs.push_back(Args.MakeArgString(
	TC.getArch() == llvm::Triple::x86
	? "-include:___asan_seh_interceptor"
	: "-include:__asan_seh_interceptor"));
	// Make sure the linker consider all object files from the dynamic runtime
	// thunk.
	CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
	TC.getCompilerRT(Args, "asan_dynamic_runtime_thunk")));
	} else if (DLL) {
	CmdArgs.push_back(TC.getCompilerRTArgString(Args, "asan_dll_thunk"));
	} else {
	for (const auto &Lib : {"asan", "asan_cxx"}) {
	CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
	// Make sure the linker consider all object files from the static lib.
	// This is necessary because instrumented dlls need access to all the
	// interface exported by the static lib in the main executable.
	CmdArgs.push_back(Args.MakeArgString(std::string("-wholearchive:") +
	TC.getCompilerRT(Args, Lib)));
	}
	}
	}

I think option 3. probably would be good to do anyway, for consistency. Yes it reduces test coverage slightly, but reallocation is less common than plain deallocation anyway, so I don’t think it’s a big deal, if we’ve already made that tradeoff for deallocation.

I suppose we can do this too. Would anyone else like to comment on this?

[mstorsjo]

Right, that block of code, adding linker flags, was iirc what I used as base when adding the same for the mingw target. But what happens if you link your executable by calling lld-link directly, which is much more common with msvc style tools? (I honestly don’t remember how asan is supposed to be used in those cases - does it pass enough arguments via directives too, or does the user of lld-link need to come up with the same as clang would produce?)

[alvinhochun]

I compiled main.obj with clang-cl -fsanitize=address /c then tried linking with lld-link and link.exe but they both couldn't link due to undefined asan-related symbols, so it looks like clang-cl did not add any linker directives to the object file.

[alvinhochun]

Patch implementing fix 1: https://reviews.llvm.org/D146908

[alvinhochun]

I realized it is still possible in some cases to trigger this issue even with fix 1, for example when the exe itself isn't instrumented and loads other DLLs which only some are instrumented with asan. I have a WIP patch https://reviews.llvm.org/D146993 to implement fix 3, but now I am not sure if this is a good idea at all, as the change is quite messy and intrusive. I may be more comfortable with declaring this scenario to be unsupported than to merge such a patch.

An alternative work around would be to use some method to inject the asan_dynamic DLL to a process on Windows. The most straightforward way is probably to patch the EXE so that an entry of the asan_dynamic DLL is prepended to the Import Directory Table.