maskit commented on code in PR #13621:
URL: https://github.com/apache/trafficserver/pull/13621#discussion_r4085903791
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1147,20 +1149,30 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader
&reader)
reader.memcpy(&buf, 1);
if (buf & 0x80) { // Insert With Name Reference
bool is_static;
- uint16_t index;
- const char *name;
- size_t name_len;
- const char *dummy;
- size_t dummy_len;
+ uint64_t index;
+ const char *name = nullptr;
+ size_t name_len = 0;
+ const char *dummy = nullptr;
+ size_t dummy_len = 0;
char *value;
size_t value_len;
if (this->_read_insert_with_name_ref(reader, is_static, index,
this->_arena, &value, value_len) < 0) {
this->_abort_decode();
return EVENT_DONE;
}
- QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%d,
value=%.*s", is_static, index, static_cast<int>(value_len),
- value);
- StaticTable::lookup(index, &name, &name_len, &dummy, &dummy_len);
+ QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%" PRIu64
", value=%.*s", is_static, index,
+ static_cast<int>(value_len), value);
+ XpackLookupResult result;
+ if (is_static) {
+ result = StaticTable::lookup(index, &name, &name_len, &dummy,
&dummy_len);
+ } else {
+ result = this->_dynamic_table.lookup_relative(index, &name, &name_len,
&dummy, &dummy_len);
+ }
+ if (result.match_type != XpackLookupResult::MatchType::EXACT) {
+ this->_arena.str_free(value);
+ this->_abort_decode();
+ return EVENT_DONE;
+ }
this->_dynamic_table.insert_entry(name, name_len, value, value_len);
Review Comment:
Confirmed. This came in with the dynamic branch; the `lookup_relative`
change I suggested kept it. On master the name always came from the static
table.
RFC 9204 §3.2.2 explicitly allows it: "A new entry can reference an entry in
the dynamic table that will be evicted when adding this new entry", and
"implementations are cautioned to avoid deleting the referenced name or value"
when that happens. Deterministic repro under ASan against a 128-byte table:
insert a 62-byte name, then a 91-byte name (this evicts the first and wraps the
storage), then insert using the name `lookup_relative(0)` returns:
```
AddressSanitizer: memcpy-param-overlap ... in XpackDynamicTableStorage::write
```
Copying the name first, the way `duplicate_entry()` does, fixes it. 47,112
randomized aliasing inserts gave no corruption and no report. Here the copy can
live in `_arena`, alongside `value`:
```cpp
char *owned_name = this->_arena.str_store(name, name_len);
this->_dynamic_table.insert_entry(owned_name, name_len, value, value_len);
this->_arena.str_free(owned_name);
```
For a regression test on the encoder stream: Set Dynamic Table Capacity 128,
then those three inserts. A conforming encoder would also wait for the earlier
inserts to be acknowledged before evicting them (§2.1.1), but the decoder
doesn't depend on that, so the test doesn't need it. It also pins relative vs
absolute, since an absolute `lookup(0)` would hit the evicted first entry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
