Page MenuHomePhabricator

[ADT] Add ASAN Support for SmallVector
Changes PlannedPublic

Authored by njames93 on Sep 7 2020, 7:37 AM.

Details

Summary

Mark addresses that are currently unused in a SmallVector as poisoned. This can help catch some iterator invalidation bugs when the SmallVector grows.

Running the ADTTest suite found an iterator invalidation bug in SmallVectorImpl<T>::insert(iterator, const T&) when the item to insert is contained inside the SmallVector.

Diff Detail

Event Timeline

njames93 created this revision.Sep 7 2020, 7:37 AM
njames93 requested review of this revision.Sep 7 2020, 7:37 AM
njames93 added inline comments.Sep 7 2020, 8:46 AM
llvm/include/llvm/ADT/SmallVector.h
594–600

Is this check the right behaviour, or is inserting an item from a SmallVector into itself considered UB?
If that would be the right behaviour then there is case to also add this check to append, push_back and resize.
If its classed as UB then the test case SmallVectorTest.PushPopTest needs addressing, specifically this line.

njames93 updated this revision to Diff 290416.Sep 8 2020, 12:28 AM

Undo fix for self referencing insertion

njames93 updated this revision to Diff 291447.Sep 13 2020, 1:06 AM

Undo SmallVectorTest fix

RKSimon added a subscriber: RKSimon.

Adding some potential reviewers

dblaikie added inline comments.
llvm/include/llvm/ADT/SmallVector.h
222

is attribute unused the right one to use here?

(also LLVM doesn't usually use top-level const on local variables)

I'd probably add a (void)Size; down next to the poison call, like we'd use with variables only used in assertions

594–600

Not sure I understand the question - Was this comment related to a different line of code?

Ah, maybe the 608 "if (I <= EltPtr", etc bit?

I think that's the one @MaskRay pointed out on the self-insert vector review, the commit message for the commit that added this support seems to indicate that the at-the-time libc++ maintainer suggested this was valid for std::vector, and was implemented in a similar way there - but it would be good to check on the implementation strategy and be able to quote some words from the standard to back this up. Could you check those things?

llvm/lib/Support/Twine.cpp
54–55

Do existing LLVM unit tests fail under ASan due to this unpoison being missing? Or is this untested? If it's untested, it'd be good to add some test coverage for it (even if those tests only fail under asan and so don't fail for most/other uses)

Similarly for the SmallString change earlier.

llvm/unittests/ADT/SmallVectorTest.cpp
256–257 ↗(On Diff #291447)

Is the intent to fix this in a later commit? Would that then make this change cause asan buildbots/builds to fail when they aren't currently? (that wouldn't be good/acceptable - so if that's the case, we'll need to decide on a fix before this is committed)

njames93 planned changes to this revision.Sep 14 2020, 2:07 AM

Adding some potential reviewers

Should point out this is current state is not intended for review, it was for context in a mailing list. Once some other issues are resolved I'll do this using the asan sequential container operations.