bneradt commented on code in PR #13603:
URL: https://github.com/apache/trafficserver/pull/13603#discussion_r4050733379
##########
src/proxy/hdrs/HTTP.cc:
##########
@@ -2262,6 +2306,8 @@ HTTPInfo::unmarshal(char *buf, int len, RefCountObj
*block_ref)
alt->m_response_hdr.m_mime = hh->m_fields_impl;
}
+ recompute_alt_wks_indices(alt);
Review Comment:
Implemented in 98f47635ca. New alternates carry a distinct marshalled tag
and an eight-byte identity covering the ordered WKS strings, MIME slot IDs, and
presence masks. Matches skip rebuilding; legacy objects and mismatches rebuild
after both heaps are swizzled. Tests cover matching and mismatching identities,
legacy objects, shared buffers, multiple alternates, and truncated trailers.
##########
src/proxy/hdrs/HdrHeap.cc:
##########
@@ -49,6 +52,95 @@ namespace
{
DbgCtl dbg_ctl_http{"http"};
+#if TS_HAS_TESTS
+// Test hook: how far to rotate the well-known string indexes written into a
marshalled heap.
+// Zero, the default, leaves marshalling alone.
+int const test_wks_idx_shift = []() -> int {
+ char const *const value = std::getenv("ATS_TEST_WKS_IDX_SHIFT");
+
+ return value != nullptr ? atoi(value) : 0;
+}();
+
+int16_t
+test_shift_wks_idx(int16_t wks_idx)
+{
+ if (wks_idx < 0) {
+ return wks_idx;
+ }
+ // Fold the configured shift into [0, hdrtoken_num_wks) here rather than
where it is read:
+ // hdrtoken_num_wks is initialized in another translation unit, so it is not
dependable during
+ // this one's static initialization. Folding also keeps a negative or
oversized environment value
+ // from producing an index that is not in the table.
+ int const shift = ((test_wks_idx_shift % hdrtoken_num_wks) +
hdrtoken_num_wks) % hdrtoken_num_wks;
+
+ return static_cast<int16_t>((wks_idx + shift) % hdrtoken_num_wks);
+}
+
+/** Make a marshalled heap look like one written by a build with a different
well-known string
+ * table: rotate every stored index, and drop the presence bits and slot
accelerators that a build
+ * lacking some of this build's strings would never have set.
+ *
+ * There is no way to run two well-known string tables in one process now that
the table is built at
+ * compile time, so this stands in for the case the reader has to survive.
Reading such a heap back
+ * has to reproduce the header the writer had, because
HTTPHdrImpl::recompute_wks_indices() rebuilds
+ * all of it from the header strings the heap also carries. See the
ATS_TEST_WKS_IDX_SHIFT autest.
+ */
+void
+test_shift_marshalled_wks_indices(HdrHeap *marshal_hdr)
+{
+ if (test_wks_idx_shift == 0) {
+ return;
+ }
+
+ char *obj_data = reinterpret_cast<char *>(marshal_hdr) + HDR_HEAP_HDR_SIZE;
+ char *heap_end = reinterpret_cast<char *>(marshal_hdr) + marshal_hdr->m_size;
+
+ while (obj_data < heap_end) {
+ HdrHeapObjImpl *obj = reinterpret_cast<HdrHeapObjImpl *>(obj_data);
+
+ switch (static_cast<HdrHeapObjType>(obj->m_type)) {
+ case HdrHeapObjType::URL: {
+ URLImpl *url = reinterpret_cast<URLImpl *>(obj);
+ url->m_scheme_wks_idx = test_shift_wks_idx(url->m_scheme_wks_idx);
+ break;
+ }
+ case HdrHeapObjType::HTTP_HEADER: {
+ HTTPHdrImpl *hh = reinterpret_cast<HTTPHdrImpl *>(obj);
+ if (hh->m_polarity == HTTPType::REQUEST) {
+ hh->u.req.m_method_wks_idx =
test_shift_wks_idx(hh->u.req.m_method_wks_idx);
+ }
+ break;
+ }
+ case HdrHeapObjType::FIELD_BLOCK: {
+ MIMEFieldBlockImpl *fblock = reinterpret_cast<MIMEFieldBlockImpl *>(obj);
+ for (uint32_t i = 0; i < fblock->m_freetop; ++i) {
+ MIMEField &field = fblock->m_field_slots[i];
+ if (field.is_live()) {
+ field.m_wks_idx = test_shift_wks_idx(field.m_wks_idx);
+ }
+ }
+ break;
+ }
+ case HdrHeapObjType::MIME_HEADER: {
+ MIMEHdrImpl *mh = reinterpret_cast<MIMEHdrImpl *>(obj);
+ mh->m_presence_bits = MIME_PRESENCE_NONE;
+ for (uint32_t &accelerator : mh->m_slot_accelerators) {
+ accelerator = 0xFFFFFFFF;
+ }
Review Comment:
Fixed in 98f47635ca. The test hook now rotates live fields in the inline
first block as well as overflow blocks, and writes a mismatched identity so the
reader takes the repair path. The WKS AuTest passes with the corrected hook.
##########
src/proxy/hdrs/unit_tests/test_Hdrs.cc:
##########
@@ -2902,6 +2902,198 @@ TEST_CASE("HTTPInfo::unmarshal_v24_1 frag bounds
checks", "[proxy][hdrtest][unma
}
}
+// ---------------------------------------------------------------------------
+// Well-known string index rebuilding.
+//
+// A cached object stores indexes into the well-known string table next to the
strings those
+// indexes stand for. A build whose table differs from the writer's would read
those indexes as
+// different strings, so HTTPInfo::unmarshal() rebuilds them from the strings.
+//
+// There is only one table in a process, so these tests stand in for a
differing table by rotating
+// every stored index and clearing the presence bits and slot accelerators
derived from them, the
+// way a build that lacked some of these strings would leave them.
+// ---------------------------------------------------------------------------
+namespace
+{
+int16_t
+rotate_wks_idx(int16_t wks_idx)
+{
+ return wks_idx < 0 ? wks_idx : static_cast<int16_t>((wks_idx + 7) %
hdrtoken_num_wks);
+}
+
+/// Rotate every well-known string index in one marshalled header heap.
+void
+scramble_marshalled_heap(HdrHeap *heap)
+{
+ char *obj_data = reinterpret_cast<char *>(heap) + sizeof(HdrHeap);
+ char *heap_end = reinterpret_cast<char *>(heap) + heap->m_size;
+
+ // Objects start at the marshalled heap's data offset, which marshal() sets
to the header size.
+ obj_data = reinterpret_cast<char *>(heap) +
reinterpret_cast<intptr_t>(heap->m_data_start);
+
+ while (obj_data < heap_end) {
+ HdrHeapObjImpl *obj = reinterpret_cast<HdrHeapObjImpl *>(obj_data);
+
+ REQUIRE(obj->m_length > 0);
+ switch (static_cast<HdrHeapObjType>(obj->m_type)) {
+ case HdrHeapObjType::URL: {
+ URLImpl *url = reinterpret_cast<URLImpl *>(obj);
+ url->m_scheme_wks_idx = rotate_wks_idx(url->m_scheme_wks_idx);
+ break;
+ }
+ case HdrHeapObjType::HTTP_HEADER: {
+ HTTPHdrImpl *hh = reinterpret_cast<HTTPHdrImpl *>(obj);
+ if (hh->m_polarity == HTTPType::REQUEST) {
+ hh->u.req.m_method_wks_idx =
rotate_wks_idx(hh->u.req.m_method_wks_idx);
+ }
+ break;
+ }
+ case HdrHeapObjType::FIELD_BLOCK: {
+ MIMEFieldBlockImpl *fblock = reinterpret_cast<MIMEFieldBlockImpl *>(obj);
+ for (uint32_t i = 0; i < fblock->m_freetop; ++i) {
+ MIMEField &field = fblock->m_field_slots[i];
+ if (field.is_live()) {
+ field.m_wks_idx = rotate_wks_idx(field.m_wks_idx);
+ }
+ }
+ break;
+ }
+ case HdrHeapObjType::MIME_HEADER: {
+ MIMEHdrImpl *mh = reinterpret_cast<MIMEHdrImpl *>(obj);
+ mh->m_presence_bits = MIME_PRESENCE_NONE;
+ mh->m_slot_accelerators[0] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[1] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[2] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[3] = 0xFFFFFFFF;
Review Comment:
Fixed in 98f47635ca. The unit scrambler now rotates the inline first-block
fields too, and the serialized alternate carries a mismatched identity. The
header unit tests pass.
##########
src/proxy/hdrs/unit_tests/test_Hdrs.cc:
##########
@@ -2902,6 +2902,198 @@ TEST_CASE("HTTPInfo::unmarshal_v24_1 frag bounds
checks", "[proxy][hdrtest][unma
}
}
+// ---------------------------------------------------------------------------
+// Well-known string index rebuilding.
+//
+// A cached object stores indexes into the well-known string table next to the
strings those
+// indexes stand for. A build whose table differs from the writer's would read
those indexes as
+// different strings, so HTTPInfo::unmarshal() rebuilds them from the strings.
+//
+// There is only one table in a process, so these tests stand in for a
differing table by rotating
+// every stored index and clearing the presence bits and slot accelerators
derived from them, the
+// way a build that lacked some of these strings would leave them.
+// ---------------------------------------------------------------------------
+namespace
+{
+int16_t
+rotate_wks_idx(int16_t wks_idx)
+{
+ return wks_idx < 0 ? wks_idx : static_cast<int16_t>((wks_idx + 7) %
hdrtoken_num_wks);
+}
+
+/// Rotate every well-known string index in one marshalled header heap.
+void
+scramble_marshalled_heap(HdrHeap *heap)
+{
+ char *obj_data = reinterpret_cast<char *>(heap) + sizeof(HdrHeap);
+ char *heap_end = reinterpret_cast<char *>(heap) + heap->m_size;
+
+ // Objects start at the marshalled heap's data offset, which marshal() sets
to the header size.
+ obj_data = reinterpret_cast<char *>(heap) +
reinterpret_cast<intptr_t>(heap->m_data_start);
Review Comment:
Fixed in 98f47635ca: obj_data is initialized directly from m_data_start,
without the dead assignment.
##########
src/proxy/hdrs/unit_tests/test_Hdrs.cc:
##########
@@ -2902,6 +2902,198 @@ TEST_CASE("HTTPInfo::unmarshal_v24_1 frag bounds
checks", "[proxy][hdrtest][unma
}
}
+// ---------------------------------------------------------------------------
+// Well-known string index rebuilding.
+//
+// A cached object stores indexes into the well-known string table next to the
strings those
+// indexes stand for. A build whose table differs from the writer's would read
those indexes as
+// different strings, so HTTPInfo::unmarshal() rebuilds them from the strings.
+//
+// There is only one table in a process, so these tests stand in for a
differing table by rotating
+// every stored index and clearing the presence bits and slot accelerators
derived from them, the
+// way a build that lacked some of these strings would leave them.
+// ---------------------------------------------------------------------------
+namespace
+{
+int16_t
+rotate_wks_idx(int16_t wks_idx)
+{
+ return wks_idx < 0 ? wks_idx : static_cast<int16_t>((wks_idx + 7) %
hdrtoken_num_wks);
+}
+
+/// Rotate every well-known string index in one marshalled header heap.
+void
+scramble_marshalled_heap(HdrHeap *heap)
+{
+ char *obj_data = reinterpret_cast<char *>(heap) + sizeof(HdrHeap);
+ char *heap_end = reinterpret_cast<char *>(heap) + heap->m_size;
+
+ // Objects start at the marshalled heap's data offset, which marshal() sets
to the header size.
+ obj_data = reinterpret_cast<char *>(heap) +
reinterpret_cast<intptr_t>(heap->m_data_start);
+
+ while (obj_data < heap_end) {
+ HdrHeapObjImpl *obj = reinterpret_cast<HdrHeapObjImpl *>(obj_data);
+
+ REQUIRE(obj->m_length > 0);
+ switch (static_cast<HdrHeapObjType>(obj->m_type)) {
+ case HdrHeapObjType::URL: {
+ URLImpl *url = reinterpret_cast<URLImpl *>(obj);
+ url->m_scheme_wks_idx = rotate_wks_idx(url->m_scheme_wks_idx);
+ break;
+ }
+ case HdrHeapObjType::HTTP_HEADER: {
+ HTTPHdrImpl *hh = reinterpret_cast<HTTPHdrImpl *>(obj);
+ if (hh->m_polarity == HTTPType::REQUEST) {
+ hh->u.req.m_method_wks_idx =
rotate_wks_idx(hh->u.req.m_method_wks_idx);
+ }
+ break;
+ }
+ case HdrHeapObjType::FIELD_BLOCK: {
+ MIMEFieldBlockImpl *fblock = reinterpret_cast<MIMEFieldBlockImpl *>(obj);
+ for (uint32_t i = 0; i < fblock->m_freetop; ++i) {
+ MIMEField &field = fblock->m_field_slots[i];
+ if (field.is_live()) {
+ field.m_wks_idx = rotate_wks_idx(field.m_wks_idx);
+ }
+ }
+ break;
+ }
+ case HdrHeapObjType::MIME_HEADER: {
+ MIMEHdrImpl *mh = reinterpret_cast<MIMEHdrImpl *>(obj);
+ mh->m_presence_bits = MIME_PRESENCE_NONE;
+ mh->m_slot_accelerators[0] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[1] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[2] = 0xFFFFFFFF;
+ mh->m_slot_accelerators[3] = 0xFFFFFFFF;
+ break;
+ }
+ default:
+ break;
+ }
+ obj_data += obj->m_length;
+ }
+}
+
+void
+parse_request(HTTPHdr &hdr, std::string_view text)
+{
+ HTTPParser parser;
+
+ http_parser_init(&parser);
+ hdr.create(HTTPType::REQUEST);
+
+ char const *start = text.data();
+ char const *end = text.data() + text.length();
+
+ REQUIRE(hdr.parse_req(&parser, &start, end, true) == ParseResult::DONE);
+ http_parser_clear(&parser);
+}
+
+void
+parse_response(HTTPHdr &hdr, std::string_view text)
+{
+ HTTPParser parser;
+
+ http_parser_init(&parser);
+ hdr.create(HTTPType::RESPONSE);
+
+ char const *start = text.data();
+ char const *end = text.data() + text.length();
+
+ REQUIRE(hdr.parse_resp(&parser, &start, end, true) == ParseResult::DONE);
+ http_parser_clear(&parser);
+}
+} // anonymous namespace
+
+TEST_CASE("HTTPHdrImpl::recompute_wks_indices rebuilds from the stored
strings", "[proxy][hdrtest][wks]")
+{
+ HTTPHdr req;
+ parse_request(req, "GET /a HTTP/1.1\r\nHost: example.com\r\nCache-Control:
no-cache\r\nAccept: */*\r\n\r\n"sv);
+ req.url_get()->scheme_set(static_cast<std::string_view>(URL_SCHEME_HTTP));
+
+ // Everything the header derives from the table is now wrong, as it would be
had it come from a
+ // build whose table differed.
+ req.m_http->u.req.m_method_wks_idx =
rotate_wks_idx(req.m_http->u.req.m_method_wks_idx);
+ req.m_http->u.req.m_url_impl->m_scheme_wks_idx =
rotate_wks_idx(req.m_http->u.req.m_url_impl->m_scheme_wks_idx);
+ req.m_mime->m_presence_bits = MIME_PRESENCE_NONE;
+ for (MIMEFieldBlockImpl *fblock = &req.m_mime->m_first_fblock; fblock !=
nullptr; fblock = fblock->m_next) {
+ for (uint32_t i = 0; i < fblock->m_freetop; ++i) {
+ MIMEField &field = fblock->m_field_slots[i];
+ if (field.is_live()) {
+ field.m_wks_idx = rotate_wks_idx(field.m_wks_idx);
+ }
+ }
+ }
+ CHECK(req.method_get() != "GET"sv);
+ CHECK(req.presence(MIME_PRESENCE_CACHE_CONTROL) == 0);
+
+ req.m_http->recompute_wks_indices();
+
+ CHECK(req.method_get() == "GET"sv);
+ CHECK(req.method_get_wksidx() == HTTP_WKSIDX_GET);
+ CHECK(req.url_get()->scheme_get() ==
static_cast<std::string_view>(URL_SCHEME_HTTP));
+ CHECK(req.presence(MIME_PRESENCE_CACHE_CONTROL) != 0);
+ CHECK(req.value_get(static_cast<std::string_view>(MIME_FIELD_CACHE_CONTROL))
== "no-cache"sv);
+ CHECK(req.value_get(static_cast<std::string_view>(MIME_FIELD_HOST)) ==
"example.com"sv);
+
+ req.destroy();
+}
+
+TEST_CASE("HTTPInfo::unmarshal rebuilds well-known string indices",
"[proxy][hdrtest][wks]")
+{
+ HTTPHdr req;
+ HTTPHdr resp;
+
+ parse_request(req, "GET /a HTTP/1.1\r\nHost: example.com\r\nAccept-Encoding:
gzip\r\n\r\n"sv);
+ req.url_get()->scheme_set(static_cast<std::string_view>(URL_SCHEME_HTTP));
+ parse_response(resp,
+ "HTTP/1.1 200 OK\r\nCache-Control:
max-age=300\r\nContent-Type: text/plain\r\nVary: Accept-Encoding\r\n\r\n"sv);
+
+ HTTPInfo info;
+ info.create();
+ info.request_set(&req);
+ info.response_set(&resp);
+
+ int const len = info.marshal_length();
+ // uint64_t elements so the buffer meets the alignment marshal() asserts on.
+ std::vector<uint64_t> storage((len + sizeof(uint64_t) - 1) /
sizeof(uint64_t), 0);
+ char *const buf = reinterpret_cast<char *>(storage.data());
+
+ REQUIRE(info.marshal(buf, len) <= len);
+
+ HTTPCacheAlt *marshalled = reinterpret_cast<HTTPCacheAlt *>(buf);
+ REQUIRE(marshalled->m_request_hdr.m_heap != nullptr);
+ REQUIRE(marshalled->m_response_hdr.m_heap != nullptr);
+ scramble_marshalled_heap(reinterpret_cast<HdrHeap *>(buf +
reinterpret_cast<intptr_t>(marshalled->m_request_hdr.m_heap)));
+ scramble_marshalled_heap(reinterpret_cast<HdrHeap *>(buf +
reinterpret_cast<intptr_t>(marshalled->m_response_hdr.m_heap)));
+
+ REQUIRE(HTTPInfo::unmarshal(buf, len, nullptr) > 0);
+
+ HTTPInfo got;
+ REQUIRE(got.get_handle(buf, len) > 0);
+
+ HTTPHdr *got_req = got.request_get();
+ HTTPHdr *got_resp = got.response_get();
+
+ CHECK(got_req->method_get() == "GET"sv);
+ CHECK(got_req->method_get_wksidx() == HTTP_WKSIDX_GET);
+ CHECK(got_req->url_get()->scheme_get() ==
static_cast<std::string_view>(URL_SCHEME_HTTP));
+
CHECK(got_req->value_get(static_cast<std::string_view>(MIME_FIELD_ACCEPT_ENCODING))
== "gzip"sv);
+ CHECK(got_req->presence(MIME_PRESENCE_HOST) != 0);
+
+
CHECK(got_resp->value_get(static_cast<std::string_view>(MIME_FIELD_CACHE_CONTROL))
== "max-age=300"sv);
+ CHECK(got_resp->value_get(static_cast<std::string_view>(MIME_FIELD_VARY)) ==
"Accept-Encoding"sv);
+ CHECK(got_resp->presence(MIME_PRESENCE_CACHE_CONTROL) != 0);
+ CHECK(got_resp->presence(MIME_PRESENCE_VARY) != 0);
+ // The cooked Cache-Control cache is keyed by directive name, so it survives
independently, but
+ // it has to still agree with the rebuilt indices.
+ CHECK(got_resp->get_cooked_cc_mask() & MIME_COOKED_MASK_CC_MAX_AGE);
+ CHECK(got_resp->get_cooked_cc_max_age() == 300);
+
+ req.destroy();
+ resp.destroy();
Review Comment:
Fixed in 98f47635ca: the test now destroys info before leaving.
--
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]
