[libcxx-commits] [PATCH] D132769: [2b/3][ASan][libcxx] std::basic_string annotations

Sat Feb 25 03:23:04 PST 2023

philnik added a subscriber: ldionne.
philnik added inline comments.

================
Comment at: libcxx/include/__string/extern_template_lists.h:31-33
+// TODO LLVM18: Remove the special-casing (_LIBCPP_CLANG_VER)
+// This change is relevant to string annotations, which require updated version of ASan API,
+// introduced in https://reviews.llvm.org/rGdd1b7b797a116eed588fd752fbe61d34deeb24e4
----------------
This comment is wrong.

================
Comment at: libcxx/include/__string/extern_template_lists.h:30
 // must never be removed from the stable list.
+#if !defined(_LIBCPP_HAS_NO_ASAN) && (_LIBCPP_CLANG_VER >= 1600)
+// TODO LLVM18: Remove the special-casing (_LIBCPP_CLANG_VER)
----------------
AdvenamTacet wrote:
> philnik wrote:
> > Why is this required?
> We don't really want to use external templates here. Those we cannot control.
> 
> Without that casing, non-instrumented functions may be used. This leads to false positives. With that change, during compilation, a version with instrumentation is created. I never had a problem of missing instrumentation after that change.
> 
> Now when I look at it, `&& (_LIBCPP_CLANG_VER >= 1600)` should be removed, same problem may happen with older LLVM.
IMO the right thing here would be to have an instrumented library, since the same problem exists for any other externally defined function, but I guess I'm OK with this for now. @ldionne are you OK with this?

================
Comment at: libcxx/include/string:1853
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_new(size_type __current_size) const _NOEXCEPT {
+      if (!__libcpp_is_constant_evaluated() && (__annotate_short_string_check() || __is_long()))
+        __annotate_contiguous_container(data(), data() + capacity() + 1, data() + capacity() + 1, data() + __current_size + 1);
----------------
Isn't this redundant? `__annotate_short_string_check()` already checks this.

================
Comment at: libcxx/include/string:3232-3233
         {
+        if (__n2 > __n1)
+          __annotate_increase(__n2 - __n1);
             size_type __n_move = __sz - __pos - __n1;
----------------
The indentation is wrong.

================
Comment at: libcxx/include/string:867
 #  endif
-      : __r_(std::move(__str.__r_)) {
+      : __r_( (__str.__annotate_delete(), std::move(__str.__r_)) ) {
     __str.__default_init();
----------------
AdvenamTacet wrote:
> AdvenamTacet wrote:
> > philnik wrote:
> > > The `__annotate_delete()` shouldn't be necessary here, right? The function is already marked `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS`, so it should be enough to call `__annotate_delete()` right before `__default_init()` if it's needed at all.
> > Unfortunately not. Sometimes instrumentation is turned on here. I tried that kind of solution first.
> > 
> > During las year I saw a lot of inconsistent behavior of ASan with variable initialization in constructors (`... : var_name(value)`) and turning off instrumentations (`_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS`).
> > 
> > In that case, it's necessary  with `test_allocator` (and probably some other of non-zero size, but I didn't confirm). Sometimes that kind of initialization is instrumented, even when function is not.
> > 
> > If we move it to the first line of the functions body (`__r_ = std::move(__str.__r_);`), then you are right.
> > And then, `__annotate_delete()` does not have to be called every time.
> > 
> > But with unpoisoning every time, `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS` seems to be not necessary.
> But `__annotate_delete()` should be only called if `__str` is short. 
I'm not a huge fan of this construct, but unfortunately I don't have a good suggestion right now. Let's at least add a comment explaining why it's done this way.

================
Comment at: libcxx/include/string:3353
+      // after __grow_by and correct state is necessery when changing annotations.
+      // __null_terminate_at is changing annotations.
+      traits_type::assign(__p + __pos, __n2, __c);
----------------
AdvenamTacet wrote:
> philnik wrote:
> > Same question here: Which invariants are not met after calling `__grow_by`? (And maybe add a comment to `__grow_by` to mention that it fails to keep the invariants)
> It's not about strings invariants, but ASan annotations. After `__growby`, those are  different than size (as `__growby` does not set size, but extends annotations), setting size just after `__grow_by` is enough to make it work and it's probably a nicer decision.
I think the annotations in `__grow_by` are wrong. Since `__grow_by` doesn't change the size of the string, it should annotate the memory to be `__old_sz` large, not that all the memory is available. That should fix the inconsistency here. Then you can just call `__null_terminate_at` before `traits_type::assign` and everything should be working properly. This would then also catch problems in a user-defined `traits_type::assign`.

================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string_string.pass.cpp:42
   assert(lhs + rhs == x);
+  assert(is_string_asan_correct(lhs + rhs));
 }
----------------
All these checks are libc++-specific, so we should mark them that way.

================
Comment at: libcxx/test/support/asan_testing.h:35
+template <typename S>
+bool __is_string_short(S const& s) {
+  // We do not have access to __is_long(), but we can check if strings
----------------

================
Comment at: libcxx/test/support/asan_testing.h:48
+TEST_CONSTEXPR bool is_string_asan_correct(const std::basic_string<ChrT, TraitsT, Alloc>& c) {
+  if (std::__libcpp_is_constant_evaluated())
+    return true;
----------------

================
Comment at: libcxx/test/support/asan_testing.h:51
+  if (c.data() != NULL) {
+#if _LIBCPP_CLANG_VER < 16000
+    // TODO LLVM18: remove special case
----------------

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132769/new/

https://reviews.llvm.org/D132769