[libcxx-commits] [PATCH] D96842: [dfsan] Do not specialize vector<bool> for DFSan
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Feb 17 12:12:06 PST 2021
ldionne added a comment.
The reason why we can't make this change as-is is that it modifies the API of `std::vector` when instantiated with `bool` when the dataflow sanitizer is used. Specifically, the specialization of `vector<bool>` has a different interface, it's not only an optimization. For example `std::vector<bool>::reference` has a `.flip()` method. So if someone is doing something like `v.flip()` (which is legal), now their code won't compile when they turn on the dataflow sanitizer. That's not acceptable, and it would make us non-conforming when the dataflow sanitizer is used.
Frankly, from your description, it sounds like the dataflow sanitizer is just not clever enough (yet?) to understand our code. This isn't specific to `std::vector` - you'll run into similar issues in a bunch of places in user code. libc++ is hardly the only place in the C++ world to do these kinds of bit manipulations. Now, this is the standard library, so we definitely want to be team players when it comes to enabling useful tooling - that's why we have things like `_LIBCPP_NO_CFI` to turn off some sanitizers in specific areas of our code where the sanitizers are known to be wrong.
Are there ways of locally turning off the dataflow sanitizer? Is there some sort of annotation that we could use to tell the dataflow sanitizer to track at the bit level for some operations inside `vector<bool>`? Those are the kinds of solutions that would be acceptable.
Also, independently of the dataflow sanitizer, it looks to me like we do have UB right here:
template <class _Allocator>
vector<bool, _Allocator>::push_back(const value_type& __x)
if (this->__size_ == this->capacity())
reserve(__recommend(this->__size_ + 1));
back() = __x; // UB HERE
As you mention in the great description for this review (thanks for that by the way), we call `back() = __x;`, but we might not have not initialized `back()` yet (if we're about to set the first bit in that byte). @miscco can you confirm that we're on the same page? If so, I'll send a review to fix this. I'm a bit surprised ubsan doesn't catch that, but I guess ubsan isn't a silver bullet.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits