[libcxx-commits] [PATCH] D136765: [ASan][libcxx] Annotating std::vector with all allocators

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet marked an inline comment as done.
AdvenamTacet added inline comments.


================
Comment at: libcxx/include/vector:747
     {
-
+#if  _LIBCPP_CLANG_VER >= 16000
+      if (!__libcpp_is_constant_evaluated() && __beg)
----------------
EricWF wrote:
> philnik wrote:
> > Could you explain here why this is possible in LLVM16, but not before?
> Seconded.
> 
> Also, I really don't like having the condition be conditionally compiled. It smells weird, and it looks weird, and I think it'll lead to bugs.
> 
> Can you find another way to write this?
Vectors annotations rely on  `__sanitizer_annotate_contiguous_container`, its implementation from LLVM15 (and earlier) requires that beginning of a containers data buffer is aligned to shadow granularity. Now, in effort to extend ASans capabilities, implementation of the function changed with commit rGdd1b7b797a116eed588fd752fbe61d34deeb24e4 and supports:
- unaligned beginnings of buffers,
- shared first/last granule (if memory granule is shared with a different object just //after the end of unaligned end/before the unaligned beginning//, memory of that object won't be poisoned).
Therefore, check for standard allocator is not necessary.

I don't know how to change the condition, requirements for the functions (`__sanitizer_annotate_contiguous_container`) arguments were weakened now and if older LLVM versions are supported, that check has to be still present for them. There are no new variables, just implementation changed. @EricWF Do you have an idea how it may be done in a different way?


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan.pass.cpp:39
         c.reserve(2*c.size());
-        volatile T foo = c[c.size()];    // bad, but not caught by ASAN
         ((void)foo);
----------------
EricWF wrote:
> philnik wrote:
> > Could you instead add a test that checks this case?
> This seems like it's defeating the purpose of the test?
Sure, I will add a new test and extend if around that one.


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan.pass.cpp:39
         c.reserve(2*c.size());
-        volatile T foo = c[c.size()];    // bad, but not caught by ASAN
+        volatile T foo = c[c.size() - 1];
         ((void)foo);
----------------
AdvenamTacet wrote:
> EricWF wrote:
> > philnik wrote:
> > > Could you instead add a test that checks this case?
> > This seems like it's defeating the purpose of the test?
> Sure, I will add a new test and extend if around that one.
It does change the purpose of the test, because with that patch, that access will be caught. But I will add a new test instead, as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765