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

[Louis Dionne via Phabricator via libcxx-commits]

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


================
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:
> ldionne wrote:
> > philnik wrote:
> > > 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?
> > Using ASAN is an ABI affecting change, kind of like our debug mode. Doing this will only seem to make things work, but in reality it'll cause other problems. For example, if you take a string created inside the dylib (without ASAN) and you try to use it from your ASAN-compiled user program, I assume you'll run into issues, right?
> > 
> > The solution is (quite unfortunately) to have a different built library for that configuration, unless I missed something.
> In general, ASan does accept false positives in situation when part of the program is compiled without ASan. That may happen with `std::vector` annotations already. But minimizing the risk of it is a good course of action.
> I agree that a separate built is the goal solution. I think, it may work as a temporary solution and then be changed into a separate library built. What do you think?
> 
> But I understand if you disagree, just I'm also not sure what has to be changed here to add an ASan library build to llvm. What has to be done? Where should I look to learn it?
> Please, let me know.
I believe this means that `-fsanitize=address` would have to cause a different `libc++.dylib` to be linked. So for example, the driver would add e.g. `-l c++asan` instead of `-l c++`, and we would have to produce `libc++asan.dylib` in addition to `libc++.dylib`. That's not a small change though, since it seems like this might be the first time a sanitizer has an effect on the version of libc++ we have to link against. But it might be the right way to go (probably the only way to go, really). In practice, disabling extern templates like this is only going to give users the impression that things work and then they will run into issues.

CC @vitalybuka


================
Comment at: libcxx/include/string:1838
+    // ASan: short string is poisoned if and only if this function returns true.
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __annotate_short_string_check() const _NOEXCEPT {
+      return _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED && !__libcpp_is_constant_evaluated();
----------------
Would `__annotate_short_string_check` be better called something like `__short_string_is_annotated()`?


================
Comment at: libcxx/include/string:2385
     __set_long_cap(__allocation.count);
+    __set_long_size(__old_sz);
+    __annotate_new(__old_sz);
----------------
Isn't that a pre-existing problem that should be fixed in its own patch?


================
Comment at: libcxx/include/string:2472
+  pointer __p;
+  size_type __old_size;
+  if (__is_long()) {
----------------
I feel like this function would be a lot simpler if we instead called `__annotate_contiguous_container` directly, no?


================
Comment at: libcxx/include/string:2567
+
+  if (__str_was_short && this != &__str)
+    __str.__annotate_shrink(__str_old_size);
----------------
This whole set of changes feels kind of clunky. Why don't we annotate delete and then annotate new instead? Wouldn't that get rid of the special `this` check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769