Copilot commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r4358060269
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1559,6 +1563,7 @@ QPACK::_read_insert_without_name_ref(IOBufferReader
&reader, Arena &arena, char
// Value
if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input +
input_len, _header_field_max_size, 7)) < 0 ||
tmp > 0xFFFF) {
+ arena.str_free(*name);
return -1;
}
Review Comment:
When `xpack_decode_string(...)` succeeds but `tmp > 0xFFFF`, `*value` has
already been allocated and assigned, but the code only frees `*name`. This
leaks the value allocation and also prevents rewinding both allocations.
Consider splitting the error cases: on `ret < 0`, free only `*name` (since
`*value` is untouched by contract); on `tmp > 0xFFFF`, free `*value` first and
then `*name` (reverse allocation order) before returning.
##########
include/proxy/http3/QPACK.h:
##########
@@ -83,6 +83,10 @@ class QPACK : public QUICApplication
static size_t estimate_header_block_size(const HTTPHdr &header_set);
private:
+ // Lets test_QPACK.cc reach the arena and the encoder stream handler, to
check that
+ // the arena gets back what QPACK allocates out of it.
+ friend struct QPACKTestAccess;
Review Comment:
This introduces a test-only `friend` into a production header, widening
access to QPACK internals for all builds and making the class’ encapsulation
depend on a test artifact name. Consider gating the friend declaration behind a
unit-test compile flag (e.g., `#if TS_HAS_TESTS` / `#ifdef UNIT_TEST`) or
providing a narrow, test-only accessor interface in a test hook to limit
exposure in non-test builds.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation
return this->_event;
}
+ // Delivered on an event thread, so wait for this to move rather than
sleeping
+ // a fixed time when the handler must outlive the events QPACK schedules.
+ int
+ events_seen()
+ {
+ return this->_events_seen.load();
+ }
+
+ bool
+ wait_for_events(int expected)
+ {
+ for (int i = 0; i < 5000 && this->_events_seen.load() < expected; ++i) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
+ return this->_events_seen.load() >= expected;
+ }
+
private:
- int _event = 0;
+ int _event = 0;
+ std::atomic<int> _events_seen{0};
Review Comment:
`_event` is written on an event thread and read from the test thread, but
it’s a plain `int`, which introduces a data race (and `wait_for_events()` can
return based on `_events_seen` while `_event` read is not synchronized). Make
`_event` an `std::atomic<int>` (or protect it with a mutex) and use appropriate
ordering so that seeing `_events_seen >= expected` also guarantees
`last_event()` observes the corresponding `_event` update.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation
return this->_event;
}
Review Comment:
`_event` is written on an event thread and read from the test thread, but
it’s a plain `int`, which introduces a data race (and `wait_for_events()` can
return based on `_events_seen` while `_event` read is not synchronized). Make
`_event` an `std::atomic<int>` (or protect it with a mutex) and use appropriate
ordering so that seeing `_events_seen >= expected` also guarantees
`last_event()` observes the corresponding `_event` update.
--
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]
