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

Mon Mar 6 10:30:47 PST 2023

hans added a comment.

In D136765#4171473 <https://reviews.llvm.org/D136765#4171473>, @philnik wrote:

> 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.

Combined with the fact that we've run tests for hundreds of millions of lines of code and found zero real issues, I think it's a good indication.

> 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 don't think the UB situation is that clear. If an allocator has allocated a chunk of ints, it's perfectly well defined behavior for that allocator or other code to access the whole chunk, even if parts of it is being used by a vector.

Another indication that these are not real issues is that the fix for all of them is to remove the container annotations.

One way to think about the situation is that with the standard allocator, these annotations work because the std::vector is in practice in complete control of the memory. With custom allocators that's no longer true, since the standard doesn't say what those allocators might do, and then these checks become much less precise.

> 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.

I can also see the theoretical benefit of this, and part of me likes it, but with the issues that are showing up, I'm just not sure it works out in practice. Perhaps it should be opt-in instead?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765