This is an archive of the discontinued LLVM Phabricator instance.

[ASan][libc++] Annotating std::deque with all allocators
ClosedPublic

Authored by AdvenamTacet on Mar 24 2023, 8:25 AM.

Details

Summary

This patch is part of our efforts to support container annotations with (almost) every allocator.
Annotating std::deque with default allocator is implemented in D132092.

Support in ASan API exests since rG1c5ad6d2c01294a0decde43a88e9c27d7437d157.

The motivation for a research and those changes was a bug, found by Trail of Bits, in a real code where an out-of-bounds read could happen as two strings were compared via a std::equals function that took iter1_begin, iter1_end, iter2_begin iterators (with a custom comparison function).
When object iter1 was longer than iter2, read out-of-bounds on iter2 could happen. Container sanitization would detect it.

If you have any questions, please email:

  • advenam.tacet@trailofbits.com
  • disconnect3d@trailofbits.com

Diff Detail

Event Timeline

AdvenamTacet created this revision.Mar 24 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 8:25 AM
AdvenamTacet requested review of this revision.Mar 24 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 8:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
AdvenamTacet planned changes to this revision.Mar 24 2023, 8:26 AM

I will add tests.

This update resolves merge conflicts with new head.

This update applies changes to the newest version of D132092.
It also adds tests similar to those in D136765.

I have a problem with local tests.

This updates the base commit.

AdvenamTacet added subscribers: vitalybuka, hans.
philnik added inline comments.Jun 14 2023, 9:27 AM
libcxx/include/deque
981

This needs tests to make sure that custom allocators work and that customizing __asan_annotate_container_with_allocator can be customized to disable annotations.

AdvenamTacet marked an inline comment as done.

This update adds tests for the patch.

  • Test for different allocators.
  • Test confirming that turning off annotations work.
ldionne accepted this revision.Jul 17 2023, 3:17 PM
ldionne added a subscriber: ldionne.

LGTM after comments are applied.

libcxx/test/libcxx/containers/sequences/deque/asan_turning_off.pass.cpp
20

Please use <cassert> instead.

<stdlib.h> shouldn't be necessary after my comments below.

29

Let's do new char[8 * 1024] instead (some platforms don't expose std::malloc).

33

And delete[] here.

This revision is now accepted and ready to land.Jul 17 2023, 3:17 PM
AdvenamTacet marked 3 inline comments as done.

Rebase and suggested changes.

Thank you @ldionne for the review!

This update:

  • assert.h -> cassert,
  • removes stdlib.h,
  • new char[...] instead of malloc,
  • does rebase,
  • delete[] instead of free.

@philnik is there anything more you want to see in the patch?

You can try rebasing and re-uploading the patch to see if the issues persist. I think this was a fluke fixed by 1cad87c07aab0e3052ad61953fc30d5155fba336.

Rebase

I also don't think the issue is related to the patch.

IDK why but the build was cancelled, can you ping the CI again? Sorry this is kinda complicated

CI rerun and rebase.

@ldionne, no worries, I know it and I will make it work.