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

[Vitaly Buka via libcxx-commits]

vitalybuka 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 does not help with OOBs after .c_str() or anything on "char*", which are likely more bug prone
than code operating on iterators.

> 
> > 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 string annotations currently, 

Agree it should not exist as is. We have the option to remove it or fix it.

It's unfair to say "continually caused problems".  It caused problems when enabled for the first time a couple years ago. Then it was disabled and unused. 
As soon as we started fixing it (last 1-2mo), and trying to use it, there was a sudden urge to remove it, just because I attracted attention :)

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

I am not against removing in principle.

I'd just waited a couple of weeks to think about alternatives or find volnteurs to improve. Maybe reply from @AdvenamTacet author of the feature.

E.g. alternatives:
1. Make Asan to ignore container annotation outside of  the heap. Then we don't need to remove these lines.
2. Make more of LLVM Asan-ware  (I am afraid to even think about the amount of work)
3. Force capacity > SSO with Asan. We don't need these lines then.

If no one is looking to jump in and disabled SSO with Asan, I'd like to try no.1 at least before reverting?

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

Not a problem. Forced out of string allocation upgrades them into use-after-free which are also easier and more reliable for Asan to detect.

> I don't think people usually expect such a behaviour change

This one is tricky. There is definitely code which does funny stuff when capacity < SSO.
I guess the simplest way is to ensure capacity is always > SSO.

We build and test a lot of strange code internally, including open source code. we can set up experiments to see how big the issue is.



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