[libcxx-commits] [libcxx] [llvm] [libcxx] Remove ASan container overflow checks for SSO strings (PR #194208)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> > > There is always condition like this in original code:
> > 
> > 
> > The GlobalOpt case I looked at that was caused by the JTS enablement was not of this form, and there was no guarantee the pointer taken into the function that was being speculatively loaded was a stack allocation (as far as I could tell).
> 
> WDYM? GlobalOpt is about globals? Applies the same logic as to the stack.
> 
> Only the heap is special. For heap objects SSO OOB detection should behave as good as non-SSO,
> 
> > This still seems like the wrong approach to me. We’re still playing whack-a-mole with optimizations if we do this, and it’s still fundamentally unsound, even if it happens to work in practice, which is a situation I would rather avoid.
> 
> We will lose a lot of good things if we abandon them just because they are not perfect. E.g we should abandon debug info, because it's metadata, and there is no guarantee it will be preserved?
> 
> So it's just a trade off. This is a tool that delivers value to users, detecting OOBs, but there is maintenance cost. Replacing, or having a plan at least, with something better would be nice.

We already have bounded iterators for `string`. If you're willing to build your own libc++, which you already have to in order for `string` to be annotated IIRC, you can just enable that and get all the bounds checking you ever wanted.

> It was in the source for a few years, I don't think we should rush to remove it before deciding on alternatives. If you need to enable JTS, a reasonable solution is just not enabled with Asan.

Right, and it continually caused problems. I'm not actually sure anybody is using the string annotations currently, since it requires a custom libc++ build (and the few I'm aware of that would do it didn't due to it causing problems for them). Given the small size of the patch I don't see how this is a problem. It's just a few lines that has to be modified to enable annotations again.

> > If we really want to detect OOB memory accesses for short strings under asan,
> 
> Why don't we? What is special about OOB on SSO so we don't want to detect them?
> 
> > we should probably just have an option that forces the string buffer to be out of line.
> 
> This would be awesome. Is this possible @philnik777 ?

It would be possible, but I do wonder how many other bugs this would hide (at least use-after-free/stack-use-after-return bugs). I don't think people usually expect such a behaviour change.

https://github.com/llvm/llvm-project/pull/194208