[libcxx-commits] [PATCH] D136765: [ASan][libcxx] Annotating std::vector with all allocators

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet added a comment.

> The question is what to do about it. I don't think there's an easy way for the allocator to remove the container annotations when recycling the memory?

Depends on your implementation, but you can easily unpoison memory (there are macros for it in the ASan API <https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/sanitizer/asan_interface.h>, but it should be possible to also use interface functions <https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/asan/asan_interface.inc> (not recommended)), or turn off instrumentation in a function accessing poisoned area. At the same time, I'm happy to implement an easy way to turn off annotations for specific allocators. Below details.

1. Unpoisoning:
  - The best option may be using macros for manual poisoning, as described in sanitizers wiki <https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning>, but I never used them, so I'm not sure.
  - @vitalybuka I think `__sanitizer_annotate_contiguous_container` cannot be used here, as the function checks state of shadow memory <https://github.com/llvm/llvm-project/blob/0d07922d921a1afe97d1056069b65976959a640c/compiler-rt/lib/asan/asan_poisoning.cpp#L447-L454>, it's a pain to work with and I would be happy to remove those checks, if you agree.

2. It's possible to access memory without checks by  turning off instrumentation with `__attribute__((__no_sanitize__("address")))`.

> we've run tests for hundreds of millions of lines of code and found zero real issues



> with the issues that are showing up, I'm just not sure it works out in practice. Perhaps it should be opt-in instead?

I strongly believe, it shouldn't be opt-in. We would have to educate everyone about that possibility and they would have to maintain list of allocators. With opt-out, one may just react to visible errors. Also, if in //"hundreds of millions of lines"// only 3 allocators are problematic, turning off annotations for them (or unpoisoning memory in them) seems like a small effort compared to a potential benefit of finding a security bug early. I understand that you didn't find anything yet, but it's same logic as for default allocator, and implementations with custom allocators may have bugs as well.

---

If @philnik thinks that opt-out is ok, I'm happy to add something like:

  template<class A>
  struct libcxx_asan_allocator_annotate {
      const bool value = true;
  }

and in vectors member function <https://github.com/llvm/llvm-project/blob/a9356a515b5a1a3637eaf5820fc0d2c0dad21a64/libcxx/include/vector#L747> `__annotate_contiguous_container` add to the check `&& libcxx_asan_allocator_annotate<_Allocator>::value`.

- @philnik what file would be best for that class definition? Probably `std::basic_string` and `std::deque` should have access to it as well.
- @hans do you have a minimal failing example? Would be good to add unit test and make sure that it solves problems you observe.

At the same time, if simply unpoisoning memory works for you, it is probably a better choice as that change increases chance of detecting bugs.

---

@ayzhao I'm not sure what exactly is happening in your case, but there is a write to memory.

> Strangely enough, calling `__asan_unpoison_memory_region(...)` in `deallocate(...)` doesn't resolve this...

Is it the first thing you do in deallocator? Are you unpoisoning whole memory for sure?

> I'm guessing the reason is because std::vector calls `__annotate_delete()` before `deallocate()`

It should leave memory poisoned in the very same way as before a constructor. Why do you think it's the reason?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136765/new/

https://reviews.llvm.org/D136765