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

Sat Feb 25 18:33:32 PST 2023

AdvenamTacet added inline comments.

================
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);
----------------
philnik wrote:
> 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`.
Ugh... my original comment was correct, please ignore my previous response. Sorry for confusion.

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

I agree, it makes more sense. I changed it. Now it works like that.

Btw. previous implementation didn't just unpoison whole memory. as `__grow_by` knows how many elements will be added, it unpoisoned exact number of bytes, so the buffer were ready.

---

The problem is with size value. In original implementation, `__grow_by` does not call  `__set_long_size`, therefore if it's called in a short string, size value is no longer correct.
- I added a call to `__set_long_size` inside `__grow_by`.
- Fixed how it annotates buffers (1).
- I made minimal changes to functions using `__grow_by` (now diff with master is much smaller).

The real problem was later in `__annotate_increace(n)` called by `__null_terminate_at`, which requires a correct size value.

---

I see that my last patch failed, but it should be easy to fix, I had to make a mistake with updating some value.
Somewhere inside `::assign(size_type __n, value_type __c)`.

```
$ "/usr/bin/python3.10" "/home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/libcxx/test/../utils/run.py" "--execdir" "/home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/test/std/strings/basic.string/string.modifiers/string_assign/Output/size_char.pass.cpp.dir" "--" "/home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/test/std/strings/basic.string/string.modifiers/string_assign/Output/size_char.pass.cpp.dir/t.tmp.exe"
# command stderr:
AddressSanitizer: CHECK failed: asan_poisoning.cpp:454 "((*(u8 *)MemToShadow(a))) == ((0))" (0x2, 0x0) (tid=354701)
    #0 0x5651a5546f51 in __asan::CheckUnwind() asan_rtl.cpp.o
    #1 0x5651a555ea32 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (/home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/test/std/strings/basic.string/string.modifiers/string_assign/Output/size_char.pass.cpp.dir/t.tmp.exe+0xdaa32) (BuildId: 05ded1d03f29cb11827d1b47b24d2ce97422315a)
    #2 0x5651a55402ca in __sanitizer_annotate_contiguous_container (/home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/test/std/strings/basic.string/string.modifiers/string_assign/Output/size_char.pass.cpp.dir/t.tmp.exe+0xbc2ca) (BuildId: 05ded1d03f29cb11827d1b47b24d2ce97422315a)
    #3 0x5651a557cfbb in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__annotate_contiguous_container[abi:v170000](void const*, void const*, void const*, void const*) const /home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1/string:1841:11
    #4 0x5651a557f60a in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__annotate_shrink[abi:v170000](unsigned long) const /home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1/string:1874:9
    #5 0x5651a557c32d in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__null_terminate_at[abi:v170000](char*, unsigned long) /home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1/string:2050:9
    #6 0x5651a557b48f in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::assign(unsigned long, char) /home/libcxx-builder/.buildkite-agent/builds/356fcbab9cbd-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1/string:2608:12
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769