brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r4358192276
##########
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:
That arm is not reachable after a successful decode. The instruction is
copied into `input[16384]` first (`QPACK.cc:1551`), `xpack_decode_string()`
rejects an encoded length longer than what remains (`XPACK.cc:123`), and a
huffman string decodes into at most twice its encoded length (`XPACK.cc:133`).
So whenever `ret >= 0`, `tmp` is below `0xFFFF`, and the only way into this
branch is `ret < 0`, where `value` was never allocated and `name` is the most
recent allocation. Splitting it would add a branch no input can reach; the same
holds for the name check above it and the value check in
`_read_insert_with_name_ref()`, so I am leaving all three as they are.
##########
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:
Leaving it ungated, which is how the tree already does this: `friend struct
RamCacheCLFUSTestAccess` (`src/iocore/cache/RamCacheCLFUS.h:123`), `friend
struct SSLNetVConnectionAsyncEpTestAccess`
(`src/iocore/net/P_SSLNetVConnection.h:120`) and `friend class
TestableChunkedHandler` (`include/proxy/http/HttpTunnel.h:74`). A friend
declaration adds no code and does not change the layout, and `QPACKTestAccess`
is only defined in `test_QPACK.cc`. `TS_HAS_TESTS` follows
`BUILD_REGRESSION_TESTING`, while `test_qpack` is built under `BUILD_TESTING`,
so gating on it would break the test in builds that turn regression tests off.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation
return this->_event;
}
Review Comment:
Made `_event` a `std::atomic<int>` in 6a36268c19. The cases added here read
it only after `wait_for_events()` sees `_events_seen` move, and that seq_cst
counter already orders the two, but the existing cases that `sleep()` and then
call `last_event()` had nothing ordering the read.
##########
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:
Same change as above, 6a36268c19.
--
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]
