Add memory safety guard on SpdyHeaderBlock iterator
This CL is the result of a multi-day investigation into a test crash. The test was the QUIC end-to-end test, and it had the following line:
EXPECT_EQ("200", client_->response_headers()->find(":status")->second);
Since response_headers() and find() are const, I had a hard time understanding why this line was corrupting memory. As it turns out, dereferencing a SpdyHeaderBlock::iterator actually modifies internal state in order to collapse all header fragments. Except if this is called on invalid memory, the collapsing can do significant damage. So in this test, if the response header did not contain the ":status" header, then it would corrupt memory.
This CL adds a mechanism to help detect this kind of bug, only in debug or asan builds. This CL does not modify behavior at all for opt builds, and does not modify behavior for well-formed programs in debug or asan.
The mechanism works by observing when the SpdyHeaderBlock is about to return an iterator that is out of bounds (equal to end()) , and setting a bool that will be checked on dereference.
This solution might be considered slightly overkill, but I really think it will save debugging time down the road.
Debug/asan-only change
PiperOrigin-RevId: 319248515
Change-Id: I2dc60b48efc97e4b3d1749c5bdae5d53de7cf48f
diff --git a/spdy/core/spdy_header_block.h b/spdy/core/spdy_header_block.h
index 5ab74e4..2348551 100644
--- a/spdy/core/spdy_header_block.h
+++ b/spdy/core/spdy_header_block.h
@@ -27,6 +27,14 @@
class ValueProxyPeer;
} // namespace test
+#ifndef SPDY_HEADER_DEBUG
+#if !defined(NDEBUG) || defined(ADDRESS_SANITIZER)
+#define SPDY_HEADER_DEBUG 1
+#else // !defined(NDEBUG) || defined(ADDRESS_SANITIZER)
+#define SPDY_HEADER_DEBUG 0
+#endif // !defined(NDEBUG) || defined(ADDRESS_SANITIZER)
+#endif // SPDY_HEADER_DEBUG
+
// This class provides a key-value map that can be used to store SPDY header
// names and values. This data structure preserves insertion order.
//
@@ -120,7 +128,12 @@
// This will result in memory allocation if the value consists of multiple
// fragments.
- const_reference operator*() const { return it_->second.as_pair(); }
+ const_reference operator*() const {
+#if SPDY_HEADER_DEBUG
+ CHECK(!dereference_forbidden_);
+#endif // SPDY_HEADER_DEBUG
+ return it_->second.as_pair();
+ }
const_pointer operator->() const { return &(this->operator*()); }
bool operator==(const iterator& it) const { return it_ == it.it_; }
@@ -137,8 +150,15 @@
return ret;
}
+#if SPDY_HEADER_DEBUG
+ void forbid_dereference() { dereference_forbidden_ = true; }
+#endif // SPDY_HEADER_DEBUG
+
private:
MapType::const_iterator it_;
+#if SPDY_HEADER_DEBUG
+ bool dereference_forbidden_ = false;
+#endif // SPDY_HEADER_DEBUG
};
typedef iterator const_iterator;
@@ -158,17 +178,17 @@
// keys and values.
std::string DebugString() const;
- iterator begin() { return iterator(map_.begin()); }
- iterator end() { return iterator(map_.end()); }
- const_iterator begin() const { return const_iterator(map_.begin()); }
- const_iterator end() const { return const_iterator(map_.end()); }
+ iterator begin() { return wrap_iterator(map_.begin()); }
+ iterator end() { return wrap_iterator(map_.end()); }
+ const_iterator begin() const { return wrap_const_iterator(map_.begin()); }
+ const_iterator end() const { return wrap_const_iterator(map_.end()); }
bool empty() const { return map_.empty(); }
size_t size() const { return map_.size(); }
iterator find(quiche::QuicheStringPiece key) {
- return iterator(map_.find(key));
+ return wrap_iterator(map_.find(key));
}
const_iterator find(quiche::QuicheStringPiece key) const {
- return const_iterator(map_.find(key));
+ return wrap_const_iterator(map_.find(key));
}
void erase(quiche::QuicheStringPiece key);
@@ -240,6 +260,31 @@
private:
friend class test::SpdyHeaderBlockPeer;
+ inline iterator wrap_iterator(MapType::const_iterator inner_iterator) const {
+#if SPDY_HEADER_DEBUG
+ iterator outer_iterator(inner_iterator);
+ if (inner_iterator == map_.end()) {
+ outer_iterator.forbid_dereference();
+ }
+ return outer_iterator;
+#else // SPDY_HEADER_DEBUG
+ return iterator(inner_iterator);
+#endif // SPDY_HEADER_DEBUG
+ }
+
+ inline const_iterator wrap_const_iterator(
+ MapType::const_iterator inner_iterator) const {
+#if SPDY_HEADER_DEBUG
+ const_iterator outer_iterator(inner_iterator);
+ if (inner_iterator == map_.end()) {
+ outer_iterator.forbid_dereference();
+ }
+ return outer_iterator;
+#else // SPDY_HEADER_DEBUG
+ return iterator(inner_iterator);
+#endif // SPDY_HEADER_DEBUG
+ }
+
void AppendHeader(const quiche::QuicheStringPiece key,
const quiche::QuicheStringPiece value);
quiche::QuicheStringPiece WriteKey(const quiche::QuicheStringPiece key);
