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

Mon Mar 6 06:50:05 PST 2023

philnik added a comment.

In D136765#4171295 <https://reviews.llvm.org/D136765#4171295>, @hans wrote:

> We've hit another issue after this in Chromium: http://crbug.com/1420967. Also, Google internal testing hit a similar issue in MySQL. That means at least three false positives after this patch. At the same time this patch has revealed zero real issues in our code.
>
> I understand the desire to make the behavior with custom allocators match that of standard allocators, but perhaps these stats indicate that the new behavior is doing more harm than good? As opposed to standard allocators, it seems that with custom allocators, it's common to have accesses to the memory that fall outside the container range.
>
> I think we should consider backing this out, and leaving the container annotations to the standard allocators only.

3 issues doesn't seem like a lot of data to build on. I don't think that these should be described as "not real issues", since they are clearly UB, which is something ASan is supposed to flag. I guess these are three different custom allocators that try to access allocated memory for some reason? I'd be fine with an opt-out mechanism for these allocators, but I think we should enable poisoning by default. e.g. for the wink-out mechanism it makes sense to poison the memory and when winking-out the objects un-poison everything. This allows ASan to still detect any overflow issues and having (most of) the performance benefit. There are also super simple allocators out there for logging etc. which would benefit from this kind of poisoning.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765