Page MenuHomePhabricator

[2a/3][ASan][libcxx] std::deque annotations
Needs ReviewPublic

Authored by AdvenamTacet on Aug 17 2022, 6:19 PM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Summary

This commit is a part of patches extending AddressSanitizer
C++ container overflow detection capabilities by adding annotations,
similar to those existing in std::vector, to std::string and
std::deque collections. These changes allow ASan to detect cases
when the instrumented program accesses memory which is internally
allocated by the collection but is still not in-use
(accesses before or after the stored elements for std::deque,
or between the size and capacity bounds for std::string).

Trail of Bits developed this as part of a research project where
we tried to find bugs using the oss-fuzz
(using llvm-14 with our modifications) harnesses.
Now, we want to upstream the llvm-15 port of this work.

That commit adds ASan annotations to std::deque.
Every std::deque chunk (contiguous memory block used by the container)
is annotated separately and API functions from [1a/3]
are used for that purpose.

Regarding performance, the introduced changes only affect cases when
a std::deque object is created or has an element added or removed.
It is similar to std::vector in that aspect.

The commit also adds unit tests for those annotations and
a compiler-rt function,
__sanitizer_verify_de_contiguous_container,
which is used within those tests.
This function can also be used in the future to verify
the sanitization/annotation verification of other
double ended contiguous containers.

Also please note that
the is_de_contiguous_container_asan_correct function
can only verify std::deque objects whose memory is never poisoned
by elements inside.
Therefore it cannot be and is not used within the tests with
a sanitized std::basic_string implementation
(which is added in the next commit in this patch series).

Structure of our patches:
[1a/3][ASan][compiler-rt] API for double ended containers
[1b/3][ASan][compiler-rt] API for annotating objects memory
[2a/3][ASan][libcxx] std::deque annotations
[2b/3][ASan][libcxx] std::basic_string annotations
[3/3][ASan] integration test for std::deque and std::basic_string annotations

1a -> 2a -

\
 --> 3
/

1b -> 2b -

If you have any questions, please email:

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

Diff Detail

Event Timeline

AdvenamTacet created this revision.Aug 17 2022, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 6:19 PM
AdvenamTacet requested review of this revision.Aug 17 2022, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 6:19 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This comment was removed by AdvenamTacet.
philnik requested changes to this revision.Aug 17 2022, 6:37 PM
philnik added a subscriber: philnik.

Thanks for working on this! I haven't done a thorough review, but here are a few style issues. FYI this patch will merge-conflict with D132081.

libcxx/include/deque
1284

Please use using instead of typedef through out.

1534

Please clang-format the new code.

1541

Please use _LIBCPP_HIDE_FROM_ABI instead.

1556

You are missing a _LIBCPP_HIDE_FROM_ABI here.

1595

The same through out.

1788

Every implementation-detail symbol should either be _Uglified or __uglified, but never __Uglified.

1847

Please add an end comment to longer preprocessor blocks.

This revision now requires changes to proceed.Aug 17 2022, 6:37 PM
AdvenamTacet marked 7 inline comments as done.

The udpate is fixing issues mentioned in the code review.
Content:

  • fixed formating,
  • use of _LIBCPP_HIDE_FROM_ABI,
  • names and style improvements.

This update should fix failing unit tests and improve code quality.
Content:

  • Fixed includes (thx philnik for pointing that)
  • Improved variable names (more consistent)
  • Values passed to compiler-rt functions are correct now. (There

was an optimization depending on compiler-rt implementation, now
it should be correct even when implementation is extended.)

Overall, it's a very small update.

Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 6:23 AM
Herald added subscribers: Restricted Project, Enna1. · View Herald Transcript

Duplicated file removed.

This update should fix failure of C++03 tests and a format check test.

This update adds clang version check (_LIBCPP_CLANG_VER >= 16000),
because the change requires compiler-rt functions from a parent revision.

As a result, it changes implementation of libcxx/test/support/asan_testing.h.

It also removes a comment fix in compiler-rt function. I will add it to
the parent revision (D132090).

Changin _de_ in names into _double_ended_, based on a review for compiler-rt part.
There will be one more change based on the review.

clang-format (sorry for double update)

Update of API. (A change was requested in review for D132090 and this
patch is reflecting that change - only one function is in API.)

Moving libc++ code from D132090.