upstream: fix PriorityStateManager indexing #3856
Conversation
Signed-off-by: Alex Konradi <akonradi@google.com>
The code was only checking whether a vector was empty before attempting to index into it. The tests were, somehow, passing even though they were invoking undefined behavior. This adds an explicit bounds check, thus preventing undefined behavior. Signed-off-by: Alex Konradi <akonradi@google.com>
| @@ -695,7 +695,7 @@ void PriorityStateManager::updateClusterPrioritySet( | |||
| HostVectorSharedPtr hosts(std::move(current_hosts)); | |||
| LocalityWeightsMap empty_locality_map; | |||
| LocalityWeightsMap& locality_weights_map = | |||
| priority_state_.empty() ? empty_locality_map : priority_state_[priority].second; | |||
| priority_state_.size() > priority ? priority_state_[priority].second : empty_locality_map; | |||
There was a problem hiding this comment.
How do we get into this situation? Should there be some test that covers this?
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry about that. Thanks, @akonradi for catching this. The relevant test is when we clearing endpoints in this test case:
envoy/test/common/upstream/eds_test.cc
Line 552 in 01d2e16
There was a problem hiding this comment.
Yeah it seems like there should be some test that should fail w/o this fix either via assertion or something else?
There was a problem hiding this comment.
I did some digging. We caught this during the Google import because our std::vector::operator[] implementation does a bounds check, which is not in the spec. Unfortunately, there's no clang sanitizer we can use to do this here since the std::vector implementation is in code, and indexing out-of-bounds doesn't trigger an ASAN violation if the vector allocated extra space.
It looks like both libstdc++ and libc++ have their own ways to enable bounds checks. I tried enabling _GLIBCXX_DEBUG locally but it causes compile errors elsewhere that I don't want to fix in this PR. Moving forward, we should probably define both _GLIBCXX_DEBUG and _LIBCPP_DEBUG for ASAN builds, but that's going to be a future PR. Happy to open an issue if that sounds reasonable.
For now, I've added back in the ASSERT. It feels a little redundant now that the bug has been fixed but it sounds like that's preferred over doing nothing.
There was a problem hiding this comment.
OK that's fine. There is another issue already opened on using _GLIBXX_DEBUG: #2556
std::vector does not do bounds checking (even under ASAN), so add an explicit ASSERT to do the bounds check. Signed-off-by: Alex Konradi <akonradi@google.com>
The PriorityStateManager is indexing off the end of an array when the priority is large enough:
envoy/source/common/upstream/upstream_impl.cc
Line 697 in c92a301
I don't know why this was passing tests before - maybe fortuitous stack layout? Either way, indexing past the end of a
std::vectoris undefined, which is bad.Risk Level: Low
Testing: Verified that the out-of-bounds indexing is actually happening by testing with an additional assert.
Docs Changes: N/A
Release Notes: N/A