This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 50c7470e5a XPACK: Inspect _start_expanding_capacity before expanding
(#12228)
50c7470e5a is described below
commit 50c7470e5ab05f050b283cecbd76b4db54fdc41e
Author: Brian Neradt <[email protected]>
AuthorDate: Wed May 7 16:18:16 2025 -0500
XPACK: Inspect _start_expanding_capacity before expanding (#12228)
While expanding XPACK storage, crashes were happening with _old_data
being nullptr. This will likely be fixed by checking the result of
_start_expanding_capacity before trying to expand capacity.
Fixes: #12225
---
include/proxy/hdrs/XPACK.h | 15 ++++++++++++++-
src/proxy/hdrs/XPACK.cc | 7 +++++--
src/proxy/hdrs/unit_tests/test_XPACK.cc | 7 +++++++
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/include/proxy/hdrs/XPACK.h b/include/proxy/hdrs/XPACK.h
index da41b069f9..8a6cd1011c 100644
--- a/include/proxy/hdrs/XPACK.h
+++ b/include/proxy/hdrs/XPACK.h
@@ -165,7 +165,7 @@ public:
/** Begin the storage expansion phase to the @a new_max_size. */
ExpandCapacityContext(XpackDynamicTableStorage &storage, uint32_t
new_max_size) : _storage{storage}
{
- this->_storage._start_expanding_capacity(new_max_size);
+ this->_ok_to_expand =
this->_storage._start_expanding_capacity(new_max_size);
}
/** End the storage expansion phase, cleaning up the old storage memory. */
~ExpandCapacityContext() { this->_storage._finish_expanding_capacity(); }
@@ -183,8 +183,21 @@ public:
*/
uint32_t copy_field(uint32_t old_offset, uint32_t len);
+ /** Indicate whether the expansion should proceed.
+ * Return whether @a _storage indicates that the new max size is valid for
+ * expanding the storage. If not, the expansion should not proceed.
+ *
+ * @return true if the new size is valid, false otherwise.
+ */
+ bool
+ ok_to_expand() const
+ {
+ return this->_ok_to_expand;
+ }
+
private:
XpackDynamicTableStorage &_storage;
+ bool _ok_to_expand = false;
};
class XpackDynamicTable
diff --git a/src/proxy/hdrs/XPACK.cc b/src/proxy/hdrs/XPACK.cc
index 6898a36fa9..6d0cde9643 100644
--- a/src/proxy/hdrs/XPACK.cc
+++ b/src/proxy/hdrs/XPACK.cc
@@ -485,8 +485,11 @@ void
XpackDynamicTable::_expand_storage_size(uint32_t new_storage_size)
{
ExpandCapacityContext context{this->_storage, new_storage_size};
- uint32_t i = this->_calc_index(this->_entries_tail, 1);
- uint32_t end = this->_calc_index(this->_entries_head, 1);
+ if (!context.ok_to_expand()) {
+ return;
+ }
+ uint32_t i = this->_calc_index(this->_entries_tail, 1);
+ uint32_t end = this->_calc_index(this->_entries_head, 1);
for (; i != end; i = this->_calc_index(i, 1)) {
auto &entry = this->_entries[i];
entry.offset = context.copy_field(entry.offset, entry.name_len +
entry.value_len);
diff --git a/src/proxy/hdrs/unit_tests/test_XPACK.cc
b/src/proxy/hdrs/unit_tests/test_XPACK.cc
index a0eace5a5f..a7f3da0e92 100644
--- a/src/proxy/hdrs/unit_tests/test_XPACK.cc
+++ b/src/proxy/hdrs/unit_tests/test_XPACK.cc
@@ -460,6 +460,7 @@ TEST_CASE("XpackDynamicTableStorage", "[xpack]")
uint32_t reoffset2 = 0, reoffset3 = 0, reoffset4 = 0;
{
ExpandCapacityContext context{storage, 200};
+ REQUIRE(context.ok_to_expand());
reoffset2 = context.copy_field(offset2, 50);
// Note that the offsets will now be shifted, starting from 0 now.
REQUIRE(reoffset2 == 0);
@@ -478,4 +479,10 @@ TEST_CASE("XpackDynamicTableStorage", "[xpack]")
storage.read(reoffset4, &name, 25, &value, 25);
REQUIRE(memcmp(name, name4.data(), 25) == 0);
REQUIRE(memcmp(value, value4.data(), 25) == 0);
+
+ // Test shrinking capacity. This should be rejected via ok_to_expand().
+ {
+ ExpandCapacityContext context{storage, 0};
+ REQUIRE(!context.ok_to_expand());
+ } // context goes out of scope and finishes the expansion phase.
}
