From 56b1e10d8513f783ac39fff55e527690ea0fc16c Mon Sep 17 00:00:00 2001 From: Phong Nguyen Date: Mon, 21 Sep 2026 15:18:20 -0700 Subject: [PATCH 1/8] Use __atomic builtins as the freelist 128-bit CAS fallback (#13571) Platforms without an inline 128-bit CAS, such as riscv64, fail the build with "unsupported processor". Neither GCC nor LLVM emit an inline 128-bit CAS on riscv64, even with the Zacas extension, so a hand-written pointer-packing branch would be the only alternative and would depend on the kernel's virtual address width. Fall back to the __atomic builtins instead. They lower to libatomic calls, which may take internal locks; that is correct because every access to a shared head_p goes through INK_QUEUE_LD and ink_atomic_cas. Also revive the orphaned atomic list stress test as Catch2 tests and remove the dead INK_QUEUE_NT code. Fixes: #13555 Co-Authored-By: Claude Fable 5 (cherry picked from commit 70c6b2c40a2026f1d47f18f81b39267e96752454) --- cmake/Check128BitCas.cmake | 59 +++++- include/tscore/ink_atomic.h | 12 ++ include/tscore/ink_config.h.cmake.in | 1 + include/tscore/ink_queue.h | 15 +- src/traffic_layout/info.cc | 1 + src/tscore/CMakeLists.txt | 9 + src/tscore/test_atomic.cc | 218 ------------------- src/tscore/unit_tests/test_InkAtomicList.cc | 221 ++++++++++++++++++++ 8 files changed, 309 insertions(+), 227 deletions(-) delete mode 100644 src/tscore/test_atomic.cc create mode 100644 src/tscore/unit_tests/test_InkAtomicList.cc diff --git a/cmake/Check128BitCas.cmake b/cmake/Check128BitCas.cmake index 850bc398fe3..5c075eeff62 100644 --- a/cmake/Check128BitCas.cmake +++ b/cmake/Check128BitCas.cmake @@ -21,6 +21,16 @@ # # TS_HAS_128BIT_CAS # TS_NEEDS_MCX16_FOR_CAS +# TS_HAS_128BIT_CAS_LIBATOMIC +# TS_NEEDS_LIBATOMIC_FOR_CAS +# +# TS_HAS_128BIT_CAS means the 16-byte __sync builtins compile and link, which the +# compiler only allows when it can emit an inline lock-free sequence. +# +# TS_HAS_128BIT_CAS_LIBATOMIC is the fallback for targets with no inline 128-bit +# CAS (e.g. riscv64): the __atomic builtins lower to libatomic calls, which may +# be lock-based there. The __sync builtins never lower to libatomic calls, so +# the fallback has to use __atomic. The two are mutually exclusive. # set(CHECK_PROGRAM @@ -33,6 +43,22 @@ set(CHECK_PROGRAM " ) +set(CHECK_PROGRAM_ATOMIC + " + int main(void) + { + __int128_t x = 0; + __int128_t y = 0; + __atomic_load(&x, &y, __ATOMIC_SEQ_CST); + return !__atomic_compare_exchange_n(&x, &y, 10, 0, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); + } + " +) + +set(NEED_MCX16 FALSE) +set(USE_LIBATOMIC_CAS FALSE) +set(NEED_LIBATOMIC FALSE) + include(CheckCSourceCompiles) check_c_source_compiles("${CHECK_PROGRAM}" TS_HAS_128BIT_CAS) @@ -44,12 +70,41 @@ if(NOT TS_HAS_128BIT_CAS) unset(CMAKE_REQUIRED_FLAGS) endif() +if(NOT TS_HAS_128BIT_CAS) + check_c_source_compiles("${CHECK_PROGRAM_ATOMIC}" TS_HAS_128BIT_CAS_BUILTIN_ATOMIC) + if(TS_HAS_128BIT_CAS_BUILTIN_ATOMIC) + set(USE_LIBATOMIC_CAS TRUE) + else() + unset(TS_HAS_128BIT_CAS_BUILTIN_ATOMIC CACHE) + set(CMAKE_REQUIRED_LIBRARIES atomic) + check_c_source_compiles("${CHECK_PROGRAM_ATOMIC}" TS_HAS_128BIT_CAS_BUILTIN_ATOMIC) + unset(CMAKE_REQUIRED_LIBRARIES) + if(TS_HAS_128BIT_CAS_BUILTIN_ATOMIC) + set(USE_LIBATOMIC_CAS TRUE) + set(NEED_LIBATOMIC TRUE) + endif() + endif() +endif() + set(TS_NEEDS_MCX16_FOR_CAS ${NEED_MCX16} CACHE BOOL "Whether -mcx16 is needed to compile CAS" ) +set(TS_HAS_128BIT_CAS_LIBATOMIC + ${USE_LIBATOMIC_CAS} + CACHE BOOL "Whether 128-bit CAS uses the __atomic builtins as a fallback" +) + +set(TS_NEEDS_LIBATOMIC_FOR_CAS + ${NEED_LIBATOMIC} + CACHE BOOL "Whether libatomic is needed to link CAS" +) + unset(CHECK_PROGRAM) -unset(NEEDS_MCX16) +unset(CHECK_PROGRAM_ATOMIC) +unset(NEED_MCX16) +unset(USE_LIBATOMIC_CAS) +unset(NEED_LIBATOMIC) -mark_as_advanced(TS_HAS_128BIT_CAS TS_NEEDS_MCX16_FOR_CAS) +mark_as_advanced(TS_HAS_128BIT_CAS TS_NEEDS_MCX16_FOR_CAS TS_HAS_128BIT_CAS_LIBATOMIC TS_NEEDS_LIBATOMIC_FOR_CAS) diff --git a/include/tscore/ink_atomic.h b/include/tscore/ink_atomic.h index 5b4d4a01f22..e5a5fdaf1d3 100644 --- a/include/tscore/ink_atomic.h +++ b/include/tscore/ink_atomic.h @@ -69,6 +69,18 @@ ink_atomic_cas(T *mem, T prev, T next) return __sync_bool_compare_and_swap(mem, prev, next); } +#if TS_HAS_128BIT_CAS_LIBATOMIC && !TS_HAS_128BIT_CAS +// The 16-byte __sync builtins never lower to libatomic calls, so targets with +// no inline 128-bit CAS (e.g. riscv64) must use the __atomic builtins instead. +// libatomic may implement them with internal locks; see INK_QUEUE_LD. +template <> +inline bool +ink_atomic_cas<__int128_t>(__int128_t *mem, __int128_t prev, __int128_t next) +{ + return __atomic_compare_exchange_n(mem, &prev, next, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} +#endif + // ink_atomic_increment(ptr, count) // Increment @ptr by @count, returning the previous value. template diff --git a/include/tscore/ink_config.h.cmake.in b/include/tscore/ink_config.h.cmake.in index fe19d38e484..dc77e7ef0ba 100644 --- a/include/tscore/ink_config.h.cmake.in +++ b/include/tscore/ink_config.h.cmake.in @@ -139,6 +139,7 @@ const int DEFAULT_STACKSIZE = @DEFAULT_STACK_SIZE@; /* Feature Flags */ #cmakedefine01 TS_HAS_128BIT_CAS +#cmakedefine01 TS_HAS_128BIT_CAS_LIBATOMIC #cmakedefine01 TS_HAS_BACKTRACE #cmakedefine01 TS_HAS_IN6_IS_ADDR_UNSPECIFIED #cmakedefine01 TS_HAS_IP_TOS diff --git a/include/tscore/ink_queue.h b/include/tscore/ink_queue.h index a0492213df3..9cbe6c757d1 100644 --- a/include/tscore/ink_queue.h +++ b/include/tscore/ink_queue.h @@ -64,6 +64,12 @@ void ink_queue_load_64(void *dst, void *src); const volatile __int128_t iqld0 = 0; \ *(__int128_t *)&(dst) = __sync_val_compare_and_swap((__int128_t *)&(src), 0, iqld0); \ } while (0) +#elif TS_HAS_128BIT_CAS_LIBATOMIC +// On targets with no inline 128-bit CAS (e.g. riscv64) libatomic may implement +// the 16-byte __atomic builtins with internal locks. That is only correct +// because every access to a shared head_p goes through INK_QUEUE_LD and +// ink_atomic_cas, so all of them serialize on the same libatomic lock. +#define INK_QUEUE_LD(dst, src) __atomic_load((__int128_t *)&(src), (__int128_t *)&(dst), __ATOMIC_SEQ_CST) #else #define INK_QUEUE_LD(dst, src) INK_QUEUE_LD64(dst, src) #endif @@ -79,7 +85,7 @@ union head_p { #if (defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4) typedef int32_t version_type; typedef int64_t data_type; -#elif TS_HAS_128BIT_CAS +#elif TS_HAS_128BIT_CAS || TS_HAS_128BIT_CAS_LIBATOMIC typedef int64_t version_type; typedef __int128_t data_type; #else @@ -124,7 +130,7 @@ union head_p { #define SET_FREELIST_POINTER_VERSION(_x, _p, _v) \ (_x).s.pointer = _p; \ (_x).s.version = _v -#elif TS_HAS_128BIT_CAS +#elif TS_HAS_128BIT_CAS || TS_HAS_128BIT_CAS_LIBATOMIC #define FREELIST_POINTER(_x) (_x).s.pointer #define FREELIST_VERSION(_x) (_x).s.version #define SET_FREELIST_POINTER_VERSION(_x, _p, _v) \ @@ -218,12 +224,7 @@ struct InkAtomicList { uint32_t offset = 0; }; -#if !defined(INK_QUEUE_NT) #define INK_ATOMICLIST_EMPTY(_x) (!(TO_PTR(FREELIST_POINTER((_x.head))))) -#else -/* ink_queue_nt.c doesn't do the FROM/TO pointer swizzling */ -#define INK_ATOMICLIST_EMPTY(_x) (!((FREELIST_POINTER((_x.head))))) -#endif // WARNING: the "name" string is not copied, it has to be a statically-stored constant string. // diff --git a/src/traffic_layout/info.cc b/src/traffic_layout/info.cc index 491be20f0be..791b611bae1 100644 --- a/src/traffic_layout/info.cc +++ b/src/traffic_layout/info.cc @@ -169,6 +169,7 @@ produce_features(bool json) print_feature("SIZEOF_VOIDP", SIZEOF_VOIDP, json); print_feature("TS_IP_TRANSPARENT", TS_IP_TRANSPARENT, json); print_feature("TS_HAS_128BIT_CAS", TS_HAS_128BIT_CAS, json); + print_feature("TS_HAS_128BIT_CAS_LIBATOMIC", TS_HAS_128BIT_CAS_LIBATOMIC, json); print_feature("TS_HAS_TESTS", TS_HAS_TESTS, json); print_feature("TS_MAX_THREADS_IN_EACH_THREAD_TYPE", TS_MAX_THREADS_IN_EACH_THREAD_TYPE, json); print_feature("TS_MAX_NUMBER_EVENT_THREADS", TS_MAX_NUMBER_EVENT_THREADS, json); diff --git a/src/tscore/CMakeLists.txt b/src/tscore/CMakeLists.txt index f951dec2bb4..f0511177e4d 100644 --- a/src/tscore/CMakeLists.txt +++ b/src/tscore/CMakeLists.txt @@ -127,6 +127,14 @@ if(TS_HAS_128BIT_CAS AND TS_NEEDS_MCX16_FOR_CAS) target_compile_options(tscore PUBLIC "-mcx16") endif() +if(TS_NEEDS_LIBATOMIC_FOR_CAS) + target_link_libraries(tscore PUBLIC atomic) +endif() + +if(TS_HAS_128BIT_CAS_LIBATOMIC AND CMAKE_CXX_COMPILER_ID MATCHES "Clang") + target_compile_options(tscore PUBLIC "-Wno-error=atomic-alignment") +endif() + if(BUILD_SHARED_LIBS) install( TARGETS tscore @@ -146,6 +154,7 @@ if(BUILD_TESTING) unit_tests/test_HKDF.cc unit_tests/test_Histogram.cc unit_tests/test_History.cc + unit_tests/test_InkAtomicList.cc unit_tests/test_IntrusivePtr.cc unit_tests/test_List.cc unit_tests/test_MMH.cc diff --git a/src/tscore/test_atomic.cc b/src/tscore/test_atomic.cc deleted file mode 100644 index ce02c7b1d13..00000000000 --- a/src/tscore/test_atomic.cc +++ /dev/null @@ -1,218 +0,0 @@ -/** @file - - A brief file description - - @section license License - - Licensed to the Apache Software Foundation (ASF) under one - or more contributor license agreements. See the NOTICE file - distributed with this work for additional information - regarding copyright ownership. The ASF licenses this file - to you under the Apache License, Version 2.0 (the - "License"); you may not use this file except in compliance - with the License. You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - */ - -#include -#include -#include -#include -#include - -#include "tscore/ink_atomic.h" -#include "tscore/ink_queue.h" -#include "tscore/ink_thread.h" - -#ifndef LONG_ATOMICLIST_TEST - -#define MAX_ALIST_TEST 10 -#define MAX_ALIST_ARRAY 100000 -InkAtomicList al[MAX_ALIST_TEST]; -void *al_test[MAX_ALIST_TEST][MAX_ALIST_ARRAY]; -int al_done = 0; - -void * -testalist(void *ame) -{ - int me = static_cast((uintptr_t)ame); - int j, k; - for (k = 0; k < MAX_ALIST_ARRAY; k++) { - ink_atomiclist_push(&al[k % MAX_ALIST_TEST], &al_test[me][k]); - } - void *x; - for (j = 0; j < 1000000; j++) { - if ((x = ink_atomiclist_pop(&al[me]))) { - ink_atomiclist_push(&al[rand() % MAX_ALIST_TEST], x); - } - } - ink_atomic_increment(&al_done, 1); - return nullptr; -} -#endif // !LONG_ATOMICLIST_TEST - -#ifdef LONG_ATOMICLIST_TEST -/************************************************************************/ -#define MAX_ATOMIC_LISTS (4 * 1024) -#define MAX_ITEMS_PER_LIST (1 * 1024) -#define MAX_TEST_THREADS 64 -static InkAtomicList alists[MAX_ATOMIC_LISTS]; -struct listItem *items[MAX_ATOMIC_LISTS * MAX_ITEMS_PER_LIST]; - -struct listItem { - int data1; - int data2; - void *link; - int data3; - int data4; - int check; -}; - -void -init_data() -{ - int j; - int ali; - struct listItem l; - struct listItem *plistItem; - - for (ali = 0; ali < MAX_ATOMIC_LISTS; ali++) - ink_atomiclist_init(&alists[ali], "alist", ((char *)&l.link - (char *)&l)); - - for (ali = 0; ali < MAX_ATOMIC_LISTS; ali++) { - for (j = 0; j < MAX_ITEMS_PER_LIST; j++) { - plistItem = (struct listItem *)malloc(sizeof(struct listItem)); - items[ali + j] = plistItem; - plistItem->data1 = ali + j; - plistItem->data2 = ali + rand(); - plistItem->link = 0; - plistItem->data3 = j + rand(); - plistItem->data4 = ali + j + rand(); - plistItem->check = (plistItem->data1 ^ plistItem->data2 ^ plistItem->data3 ^ plistItem->data4); - ink_atomiclist_push(&alists[ali], plistItem); - } - } -} - -void -cycle_data(void *d) -{ - InkAtomicList *l; - struct listItem *pli; - struct listItem *pli_next; - int iterations; - int me; - - me = (int)d; - iterations = 0; - - while (1) { - l = &alists[(me + rand()) % MAX_ATOMIC_LISTS]; - - pli = (struct listItem *)ink_atomiclist_popall(l); - if (!pli) - continue; - - // Place listItems into random queues - while (pli) { - ink_assert((pli->data1 ^ pli->data2 ^ pli->data3 ^ pli->data4) == pli->check); - pli_next = (struct listItem *)pli->link; - pli->link = 0; - ink_atomiclist_push(&alists[(me + rand()) % MAX_ATOMIC_LISTS], (void *)pli); - pli = pli_next; - } - iterations++; - poll(0, 0, 10); // 10 msec delay - if ((iterations % 100) == 0) - printf("%d ", me); - } -} - -/************************************************************************/ -#endif // LONG_ATOMICLIST_TEST - -int -main(int /* argc ATS_UNUSED */, const char * /* argv ATS_UNUSED */[]) -{ -#ifndef LONG_ATOMICLIST_TEST - int32_t m = 1, n = 100; - // int64 lm = 1LL, ln = 100LL; - const char *m2 = "hello"; - char *n2; - - printf("sizeof(int32_t)==%d sizeof(void *)==%d\n", static_cast(sizeof(int32_t)), static_cast(sizeof(void *))); - - printf("CAS: %d == 1 then 2\n", m); - n = ink_atomic_cas(&m, 1, 2); - printf("changed to: %d, result=%s\n", m, n ? "true" : "false"); - - printf("CAS: %d == 1 then 3\n", m); - n = ink_atomic_cas(&m, 1, 3); - printf("changed to: %d, result=%s\n", m, n ? "true" : "false"); - - printf("CAS pointer: '%s' == 'hello' then 'new'\n", m2); - n = ink_atomic_cas(&m2, "hello", "new"); - printf("changed to: %s, result=%s\n", m2, n ? (char *)"true" : (char *)"false"); - - printf("CAS pointer: '%s' == 'hello' then 'new2'\n", m2); - n = ink_atomic_cas(&m2, m2, "new2"); - printf("changed to: %s, result=%s\n", m2, n ? "true" : "false"); - - n = 100; - printf("Atomic Inc of %d\n", n); - m = ink_atomic_increment(static_cast(&n), 1); - printf("changed to: %d, result=%d\n", n, m); - - printf("Atomic Fetch-and-Add 2 to pointer to '%s'\n", m2); - n2 = static_cast(ink_atomic_increment((void **)&m2, (void *)2)); - printf("changed to: %s, result=%s\n", m2, n2); - - printf("Testing atomic lists\n"); - { - int ali; - srand(time(nullptr)); - printf("sizeof(al_test) = %d\n", static_cast(sizeof(al_test))); - memset(&al_test[0][0], 0, sizeof(al_test)); - for (ali = 0; ali < MAX_ALIST_TEST; ali++) { - ink_atomiclist_init(&al[ali], "foo", 0); - } - for (ali = 0; ali < MAX_ALIST_TEST; ali++) { - ink_thread tid; - pthread_attr_t attr; - - pthread_attr_init(&attr); -#if !defined(freebsd) - pthread_attr_setstacksize(&attr, 1024 * 1024); -#endif - ink_assert(pthread_create(&tid, &attr, testalist, (void *)((intptr_t)ali)) == 0); - } - while (al_done != MAX_ALIST_TEST) { - sleep(1); - } - } -#endif // !LONG_ATOMICLIST_TEST - -#ifdef LONG_ATOMICLIST_TEST - printf("Testing atomic lists (long version)\n"); - { - int id; - - init_data(); - for (id = 0; id < MAX_TEST_THREADS; id++) { - ink_assert(thr_create(NULL, 0, cycle_data, (void *)id, THR_NEW_LWP, NULL) == 0); - } - } - while (1) { - poll(0, 0, 10); // 10 msec delay - } -#endif // LONG_ATOMICLIST_TEST - - return 0; -} diff --git a/src/tscore/unit_tests/test_InkAtomicList.cc b/src/tscore/unit_tests/test_InkAtomicList.cc new file mode 100644 index 00000000000..b7ba97fd74e --- /dev/null +++ b/src/tscore/unit_tests/test_InkAtomicList.cc @@ -0,0 +1,221 @@ +/** @file + + Concurrency stress tests for InkAtomicList and InkFreeList. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include + +#include +#include +#include +#include +#include + +#include "tscore/ink_queue.h" + +namespace +{ + +// Deterministic per-thread PRNG so failures are reproducible. +struct XorShift { + uint64_t state; + + explicit XorShift(uint64_t seed) : state(seed | 1) {} + + uint32_t + next() + { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + return static_cast(state >> 32); + } +}; + +struct Item { + Item *next = nullptr; + std::atomic in_hand{0}; + uint32_t owner = 0; + uint32_t serial = 0; + uint32_t check = 0; +}; + +constexpr int NUM_LISTS = 8; +constexpr int NUM_THREADS = 6; +constexpr int ITEMS_PER_THREAD = 2000; +constexpr int OPS_PER_THREAD = 100000; + +// Claim exclusive ownership of a popped item. A second concurrent claim means +// the same item was reachable from two places, i.e. the list is corrupt. +bool +claim(Item *item) +{ + return item->in_hand.exchange(1, std::memory_order_acq_rel) == 0; +} + +void +release(Item *item) +{ + item->in_hand.store(0, std::memory_order_release); +} + +} // end anonymous namespace + +TEST_CASE("InkAtomicList: concurrent push/pop/popall conserves items", "[libts][InkAtomicList]") +{ + InkAtomicList lists[NUM_LISTS]; + std::vector items(static_cast(NUM_THREADS) * ITEMS_PER_THREAD); + + for (int i = 0; i < NUM_LISTS; i++) { + ink_atomiclist_init(&lists[i], "test_InkAtomicList", offsetof(Item, next)); + } + + std::atomic claim_failures{0}; + std::atomic check_failures{0}; + + auto worker = [&](int me) { + XorShift rng(0x9e3779b97f4a7c15ull * (me + 1)); + + for (int k = 0; k < ITEMS_PER_THREAD; k++) { + Item *item = &items[static_cast(me) * ITEMS_PER_THREAD + k]; + item->owner = me; + item->serial = k; + item->check = item->owner ^ item->serial ^ 0xdeadbeef; + ink_atomiclist_push(&lists[k % NUM_LISTS], item); + } + + for (int op = 0; op < OPS_PER_THREAD; op++) { + InkAtomicList *l = &lists[rng.next() % NUM_LISTS]; + + if ((op & 1023) == 0) { + // Drain a whole list and scatter it back. + Item *chain = static_cast(ink_atomiclist_popall(l)); + while (chain != nullptr) { + Item *next_item = chain->next; + if (!claim(chain)) { + claim_failures++; + } + if (chain->check != (chain->owner ^ chain->serial ^ 0xdeadbeef)) { + check_failures++; + } + release(chain); + ink_atomiclist_push(&lists[rng.next() % NUM_LISTS], chain); + chain = next_item; + } + } else { + Item *item = static_cast(ink_atomiclist_pop(l)); + if (item == nullptr) { + continue; + } + if (!claim(item)) { + claim_failures++; + } + if (item->check != (item->owner ^ item->serial ^ 0xdeadbeef)) { + check_failures++; + } + release(item); + ink_atomiclist_push(&lists[rng.next() % NUM_LISTS], item); + } + } + }; + + std::vector threads; + for (int t = 0; t < NUM_THREADS; t++) { + threads.emplace_back(worker, t); + } + for (auto &t : threads) { + t.join(); + } + + REQUIRE(claim_failures == 0); + REQUIRE(check_failures == 0); + + // Every item must be reachable exactly once across all lists. + size_t drained = 0; + for (int i = 0; i < NUM_LISTS; i++) { + Item *chain = static_cast(ink_atomiclist_popall(&lists[i])); + while (chain != nullptr) { + REQUIRE(claim(chain)); + REQUIRE(chain->check == (chain->owner ^ chain->serial ^ 0xdeadbeef)); + drained++; + chain = chain->next; + } + } + REQUIRE(drained == items.size()); +} + +TEST_CASE("InkAtomicList: remove", "[libts][InkAtomicList]") +{ + InkAtomicList l; + Item items[3]; + + ink_atomiclist_init(&l, "test_InkAtomicList_remove", offsetof(Item, next)); + for (auto &item : items) { + ink_atomiclist_push(&l, &item); + } + + // Remove from the middle, the head, then a missing item. + REQUIRE(ink_atomiclist_remove(&l, &items[1]) == &items[1]); + REQUIRE(ink_atomiclist_remove(&l, &items[2]) == &items[2]); + REQUIRE(ink_atomiclist_remove(&l, &items[1]) == nullptr); + REQUIRE(ink_atomiclist_pop(&l) == &items[0]); + REQUIRE(INK_ATOMICLIST_EMPTY(l)); +} + +TEST_CASE("InkFreeList: concurrent new/free", "[libts][InkFreeList]") +{ + constexpr int SLOTS = 32; + constexpr int FL_OPS = 50000; + constexpr uint32_t OBJ_SIZE = 128; + + InkFreeList *f = ink_freelist_create("test_InkFreeList", OBJ_SIZE, 64, 8); + + auto worker = [&](int me) { + XorShift rng(0xc2b2ae3d27d4eb4full * (me + 1)); + void *slots[SLOTS] = {nullptr}; + + for (int op = 0; op < FL_OPS; op++) { + int i = rng.next() % SLOTS; + if (slots[i] != nullptr) { + ink_freelist_free(f, slots[i]); + slots[i] = nullptr; + } else { + slots[i] = ink_freelist_new(f); + memset(slots[i], me, OBJ_SIZE); + } + } + for (auto &slot : slots) { + if (slot != nullptr) { + ink_freelist_free(f, slot); + } + } + }; + + std::vector threads; + for (int t = 0; t < NUM_THREADS; t++) { + threads.emplace_back(worker, t); + } + for (auto &t : threads) { + t.join(); + } + + REQUIRE(f->used == 0); +} From 64b52b20e311e75113f1e944391195bc447c43e3 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 22 Sep 2026 09:52:57 +0200 Subject: [PATCH 2/8] QPACK: use || in the varint decode range guards (#13655) Ten guards in QPACK.cc read `xpack_decode_integer(...) < 0 && value > 0xFFFF`, so neither condition rejects anything: a decode failure falls through, and an oversized varint is silently narrowed into the surrounding uint16_t. The delta_base_index guard also had its comparison inverted. The unchecked failure matters more than the truncation. On failure xpack_decode_integer returns -1, and callers then do `read_len += ret`, giving SIZE_MAX. IOBufferReader::consume() takes that as -1, its release-assert passes because is_read_avail_more_than(-2) is true, and start_offset moves backwards. The helper returns 0 rather than a negative, so _on_encoder_stream_read_ready() does not abort and its `while (is_read_avail_more_than(0))` loop re-reads the same byte. Flip the ten operators and correct the inverted comparison. The value guard in _read_insert_with_name_ref also has its bound raised from 0xFF to 0xFFFF, matching the other nine sites. This is required rather than cosmetic: value_len is a size_t, so 0xFF bounds nothing, and under || a 0xFF bound would reject every header value longer than 255 bytes. At 0xFFFF the check is unreachable, since xpack_decode_string is already capped by _header_field_max_size. (cherry picked from commit bfe88e104a2bb174220f525436e148c45145f845) --- src/proxy/http3/QPACK.cc | 22 ++++++++-------- src/proxy/http3/test/test_QPACK.cc | 40 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 38aa3a5630f..63358997cc5 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -284,7 +284,7 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo uint64_t tmp = 0; int64_t ret = xpack_decode_integer(tmp, header_block, header_block + header_block_len, 8); - if (ret < 0 && tmp > 0xFFFF) { + if (ret < 0 || tmp > 0xFFFF) { return -1; } uint16_t largest_reference = tmp; @@ -922,7 +922,7 @@ QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTP // Decode Header Data Prefix uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, pos, pos + remain_len, 8)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, pos, pos + remain_len, 8)) < 0 || tmp > 0xFFFF) { return -1; } pos += ret; @@ -931,7 +931,7 @@ QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTP uint64_t delta_base_index; uint16_t base_index; - if ((ret = xpack_decode_integer(delta_base_index, pos, pos + remain_len, 7)) < 0 && delta_base_index < 0xFFFF) { + if ((ret = xpack_decode_integer(delta_base_index, pos, pos + remain_len, 7)) < 0 || delta_base_index > 0xFFFF) { return -2; } @@ -1514,15 +1514,15 @@ QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint1 // Name Index uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 || tmp > 0xFFFF) { return -1; } index = tmp; read_len += ret; // Value - if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 && - tmp > 0xFF) { + if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 || + tmp > 0xFFFF) { return -1; } value_len = tmp; @@ -1545,14 +1545,14 @@ QPACK::_read_insert_without_name_ref(IOBufferReader &reader, Arena &arena, char // Name uint64_t tmp; - if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 || tmp > 0xFFFF) { return -1; } name_len = tmp; read_len += ret; // Value - if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 && + if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 || tmp > 0xFFFF) { return -1; } @@ -1575,7 +1575,7 @@ QPACK::_read_duplicate(IOBufferReader &reader, uint16_t &index) // Index uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 || tmp > 0xFFFF) { return -1; } index = tmp; @@ -1597,7 +1597,7 @@ QPACK::_read_dynamic_table_size_update(IOBufferReader &reader, uint16_t &max_siz uint64_t tmp; // Max Size - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 || tmp > 0xFFFF) { return -1; } max_size = tmp; @@ -1619,7 +1619,7 @@ QPACK::_read_table_state_synchronize(IOBufferReader &reader, uint16_t &insert_co uint64_t tmp; // Insert Count - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 || tmp > 0xFFFF) { return -1; } insert_count = tmp; diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index 7059c773f94..f9c1d193d27 100644 --- a/src/proxy/http3/test/test_QPACK.cc +++ b/src/proxy/http3/test/test_QPACK.cc @@ -439,6 +439,46 @@ TEST_CASE("Encoding", "[qpack-encode]") } } +// QPACK::decode() parses the Required Insert Count varint from the header +// block prefix and stores the decoded value in a uint16_t local; the call site +// must reject a varint whose value exceeds the uint16_t range, otherwise the +// value silently truncates and the decoder proceeds with corrupted state. +// Drive QPACK::decode() with a 4-byte 8-bit-prefix varint encoding 0x10000 +// and assert that the decoder reports failure rather than accepting it. +TEST_CASE("decode() rejects oversized Required Insert Count at entry", "[qpack-decode-entry-bounds]") +{ + QUICApplicationDriver driver; + auto qpack = std::make_unique(driver.get_connection(), UINT32_MAX, 4096, 100, MAX_FIELD_SIZE); + auto handler = std::make_unique(); + + HTTPHdr hdr; + hdr.create(HTTPType::REQUEST); + + uint8_t block[16] = {0}; + uint8_t *p = block; + int enc_len = xpack_encode_integer(p, p + sizeof(block), 0x10000, 8); + REQUIRE(enc_len > 0); + p += enc_len; + *p++ = 0x00; + size_t block_len = static_cast(p - block); + + int sync_ret = qpack->decode(1, block, block_len, hdr, handler.get(), eventProcessor.all_ethreads[0]); + + // decode() schedules its result asynchronously when it returns >= 0; only + // wait in that case. A synchronous failure (sync_ret < 0) means no event + // will be delivered and there is nothing to wait for. + if (sync_ret >= 0) { + sleep(1); + } + + CAPTURE(sync_ret); + CAPTURE(handler->last_event()); + CHECK_FALSE((sync_ret == 0 && handler->last_event() == QPACK_EVENT_DECODE_COMPLETE)); + CHECK((sync_ret < 0 || handler->last_event() == QPACK_EVENT_DECODE_FAILED)); + + hdr.destroy(); +} + TEST_CASE("Decoding", "[qpack-decode]") { char app_dir[PATH_MAX + 1] = ""; From 210980c567266f01e4ed810a04e39107ef172f1a Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 22 Sep 2026 20:57:40 +0900 Subject: [PATCH 3/8] hrw4u: exit non-zero on compile errors (#13656) * hrw4u: exit non-zero on compile errors The exit gate required `tree is None`, but ANTLR error recovery almost always yields a tree, so both syntax and semantic errors exited 0 while printing diagnostics and a partial .conf. Collecting every error and failing the build were mutually exclusive: only --stop-on-error exited 1. Sandbox denials were caught by the same gate, so the "denied" outcome the sandbox docs describe also exited 0. generate_output now reports failure by return value and run_main owns the exit, so a bad file in a bulk run no longer aborts the files after it. A failing compile still prints its partial .conf; the exit code now marks it untrustworthy. Suppressing those bytes would change behavior for existing pipelines and is left as a separate decision. * hrw4u: parse real input in generate_output return-value tests The failure test passed parser_obj=None, which only worked because a None tree short-circuits before the AST branch reads it. Parsing a real input instead also pins the regression: the input parses, so the tree is not None -- exactly what the old exit gate let through. * hrw4u: cover u4wrh in the exit-code tests, scope the doc claim u4wrh drives the same run_main(), so the contract regresses just as easily there; verified the new test exits 0 against the pre-fix code. The doc said every input is processed before the status is decided, which reads as covering the fatal argument and I/O paths too -- those still exit immediately. * hrw4u: document exit status 2 for usage errors run_main() lets argparse handle the command line, so an unknown option, a bad option value, or conflicting output modes exit 2, not the 1 the table claimed. Normalizing them to 1 would merge "you typed the command wrong" into "your rules did not compile", so the doc follows the code. Row 1 now lists only what actually exits 1; the mixed bulk/stdout rejection is caught after parsing and belongs there, not with argparse. (cherry picked from commit 915543f9a5ceb0b8255413d63682ed9247a60543) --- doc/admin-guide/configuration/hrw4u.en.rst | 21 +++++ tools/hrw4u/src/common.py | 24 ++++-- tools/hrw4u/tests/test_cli.py | 91 ++++++++++++++++++++++ tools/hrw4u/tests/test_common.py | 26 +++++-- 4 files changed, 148 insertions(+), 14 deletions(-) diff --git a/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst index 049454f33c0..e387d0f2fb5 100644 --- a/doc/admin-guide/configuration/hrw4u.en.rst +++ b/doc/admin-guide/configuration/hrw4u.en.rst @@ -115,6 +115,27 @@ This is particularly useful for build systems or when processing many configurat files at once. All files are processed in a single invocation, improving performance for large batches of files. +Exit Status +^^^^^^^^^^^ + +====== ========================================================================== +Status Meaning +====== ========================================================================== +0 Every input compiled. Warnings may still have been reported. +1 At least one input had an error, or the run aborted on a fatal problem: + a missing or unreadable input, an unwritable output, or ``input:output`` + pairs mixed with plain file arguments. +2 The command line was rejected before any input was read: an unknown + option, an invalid option value, or conflicting output modes. This comes + from ``argparse`` and follows Python's usage-error convention. +====== ========================================================================== + +A compile error does not stop the run: every input is still processed before +the status is decided, so one bad file in a multi-file or bulk run does not +skip the files after it. The fatal problems above still abort immediately. A +failing compile writes its partial output; the exit status is what marks that +output untrustworthy. + Reverse Tool (u4wrh) ^^^^^^^^^^^^^^^^^^^^ diff --git a/tools/hrw4u/src/common.py b/tools/hrw4u/src/common.py index 15f1885f4bd..22ee24e7939 100644 --- a/tools/hrw4u/src/common.py +++ b/tools/hrw4u/src/common.py @@ -238,8 +238,12 @@ def generate_output( filename: str, args: Any, error_collector: ErrorCollector | None = None, - extra_kwargs: dict[str, Any] | None = None) -> None: - """Generate and print output based on mode with optional error collection.""" + extra_kwargs: dict[str, Any] | None = None) -> bool: + """Generate and print output based on mode with optional error collection. + + Returns True when the input produced errors, so the caller can set the exit + status after every input has been processed rather than aborting mid-run. + """ if args.ast: if tree is not None: print(tree.toStringTree(recog=parser_obj)) @@ -278,8 +282,8 @@ def generate_output( if error_collector and (error_collector.has_errors() or error_collector.has_warnings()): print(error_collector.get_error_summary(), file=sys.stderr) - if error_collector.has_errors() and not args.ast and tree is None: - sys.exit(1) + + return bool(error_collector and error_collector.has_errors()) def run_main( @@ -363,10 +367,12 @@ def run_main( emit_fatal_error(args.error_format, e) tree, parser_obj, error_collector = create_parse_tree( content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors, args.error_format) - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + if generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs): + sys.exit(1) return if any(':' in f for f in args.files): + failed = False for pair in args.files: if ':' not in pair: emit_fatal_message( @@ -398,12 +404,13 @@ def run_main( original_stdout = sys.stdout try: sys.stdout = output_file - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) finally: sys.stdout = original_stdout except Exception as e: emit_fatal_message(args.error_format, f"Error writing to '{output_path}': {e}", filename=output_path) else: + failed = False for i, input_path in enumerate(args.files): if i > 0: print("# ---") @@ -426,4 +433,7 @@ def run_main( content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors, args.error_format) - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + + if failed: + sys.exit(1) diff --git a/tools/hrw4u/tests/test_cli.py b/tools/hrw4u/tests/test_cli.py index 886728de052..ddb1ebcd850 100644 --- a/tools/hrw4u/tests/test_cli.py +++ b/tools/hrw4u/tests/test_cli.py @@ -48,6 +48,14 @@ def run_hrw4u(args: list[str], stdin: str | None = None) -> subprocess.Completed return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd()) +def run_u4wrh(args: list[str], stdin: str | None = None) -> subprocess.CompletedProcess: + """Run u4wrh script with given arguments.""" + script = Path("scripts/u4wrh").resolve() + cmd = [sys.executable, str(script)] + args + + return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd()) + + def test_cli_single_file_to_stdout(sample_hrw4u_files: tuple[Path, Path, Path]) -> None: """Test compiling a single file to stdout.""" file1, _, _ = sample_hrw4u_files @@ -244,3 +252,86 @@ def test_cli_help_lists_error_format_flag() -> None: assert "--error-format" in result.stdout for choice in ("plain", "json", "markdown"): assert choice in result.stdout + + +# +# Exit-code contract: a compile error must fail the build. +# + + +def test_cli_exits_nonzero_on_syntax_error(tmp_path: Path) -> None: + """A syntax error must exit non-zero even though ANTLR recovers and yields a tree.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n inbound.req.X-Foo = \n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert ": error:" in result.stderr + + +def test_cli_exits_nonzero_on_semantic_error(tmp_path: Path) -> None: + """A semantic error must exit non-zero; the parse tree exists, so only sema catches it.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n test::add-debug-header(\"foo\");\n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert "unknown procedure" in result.stderr + + +def test_cli_collects_all_errors_and_still_exits_nonzero(tmp_path: Path) -> None: + """Multi-error mode must report every diagnostic AND fail; the two are not exclusive.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n bogus.one = \"a\";\n bogus.two = \"b\";\n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert result.stderr.count(": error:") >= 2 + + +def test_cli_multi_file_exits_nonzero_if_any_fails(sample_hrw4u_files: tuple[Path, Path, Path], tmp_path: Path) -> None: + """One bad file among good ones fails the run, but the good ones are still processed.""" + good, _, _ = sample_hrw4u_files + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n test::nope(\"x\");\n}\n") + + result = run_hrw4u([str(bad), str(good)]) + + assert result.returncode != 0 + assert "no-op" in result.stdout, "processing must continue past the failing file" + + +def test_cli_u4wrh_exits_nonzero_on_error(tmp_path: Path) -> None: + """u4wrh shares run_main(), so it must honor the same exit-status contract.""" + bad = tmp_path / "bad.conf" + bad.write_text("cond %{READ_REQUEST_HDR_HOOK}\n set-header X-Foo\n") + + result = run_u4wrh([str(bad)]) + + assert result.returncode != 0 + assert ": error:" in result.stderr + + +@pytest.mark.parametrize( + "argv", [["--bogus"], ["--ast", "--hrw"], ["--error-format", "nope"], ["--max-errors", "x"]], + ids=["unknown-option", "conflicting-output-modes", "invalid-choice", "invalid-int"]) +def test_cli_usage_error_exits_two(argv: list[str]) -> None: + """Usage errors come from argparse and exit 2, not 1; the documented status must hold.""" + result = run_hrw4u(argv, stdin="") + + assert result.returncode == 2 + assert "usage:" in result.stderr + + +def test_cli_mixed_file_formats_exits_one(tmp_path: Path) -> None: + """Mixed bulk/stdout arguments are rejected by run_main() itself, so they exit 1, not 2.""" + good = tmp_path / "good.hrw4u" + good.write_text("REMAP {\n no-op();\n}\n") + + result = run_hrw4u([f"{good}:{tmp_path / 'out.conf'}", str(good)]) + + assert result.returncode == 1 + assert "Mixed formats not allowed" in result.stderr diff --git a/tools/hrw4u/tests/test_common.py b/tools/hrw4u/tests/test_common.py index d17cdf6ad5d..1997f28cbed 100644 --- a/tools/hrw4u/tests/test_common.py +++ b/tools/hrw4u/tests/test_common.py @@ -164,14 +164,26 @@ def test_ast_mode_tree_none_with_errors(self, capsys): out = capsys.readouterr().out assert "Parse tree not available" in out - def test_error_collector_exits_on_parse_failure(self, capsys): - """When tree is None and errors exist in non-AST mode, should exit(1).""" - errors = ErrorCollector() - errors.add_error(Hrw4uSyntaxError("", 1, 0, "parse failed", "bad")) + def test_error_collector_reports_failure_to_caller(self): + """generate_output reports errors via its return value; run_main owns the exit status. + + The input parses, so ``tree`` is not None -- the exact shape the old + ``tree is None`` exit gate let through. + """ + tree, parser_obj, errors = create_parse_tree( + 'REMAP { test::nope("x"); }', "", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True) args = SimpleNamespace(ast=False, debug=False, no_comments=False) - with pytest.raises(SystemExit) as exc_info: - generate_output(None, None, HRW4UVisitor, "", args, errors) - assert exc_info.value.code == 1 + + assert tree is not None + assert generate_output(tree, parser_obj, HRW4UVisitor, "", args, errors) is True + + def test_clean_input_reports_no_failure(self): + """A clean parse must report False so a multi-file run keeps exit status 0.""" + tree, parser_obj, errors = create_parse_tree( + 'REMAP { no-op(); }', "", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True) + args = SimpleNamespace(ast=False, debug=False, no_comments=False) + + assert generate_output(tree, parser_obj, HRW4UVisitor, "", args, errors) is False def test_visitor_exception_collected(self, capsys): """When visitor.visit() raises, error is collected and reported.""" From 9a31b7cae9d28b8d25fd6cca955cb0238a0a96ae Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 22 Sep 2026 20:59:41 +0900 Subject: [PATCH 4/8] hrw4u: add an AST round-trip test over the whole corpus (#13699) * hrw4u: add an AST round-trip test over the whole corpus Render every corpus input's AST back to hrw4u and require the compiled config to be unchanged: whatever the AST drops, the config loses too. Unlike hand-written cases, nothing has to be enumerated in advance. It found five losses, all fixed here: - an empty `else { }` looked like no else clause, so a sandbox policy denying 'else' was evaded by writing one - comments were discarded, though five .conf goldens carry them - a bool assignment lost the spelling the emitter echoes back - parentheses were unwrapped, dropping the cond %{GROUP} they emit - a set and an iprange both became a tuple of IPValue, though in [1.2.3.4] emits (1.2.3.4) and in {1.2.3.4} emits {1.2.3.4} IfBlock.has_else is required rather than defaulted, so a site that rebuilds the node and forgets it fails instead of reopening the bypass. A second test asserts the corpus reaches every grammar rule; a bare $param value had no fixture, now added. * hrw4u: keep bool spelling in every value context The AST kept the source spelling only on an assignment RHS, so a TRUE in a comparison or a function argument regenerated as true and changed the emitted config. bool-spelling.input.txt witnesses both; the reverse normalizes an argument's spelling, hence the exceptions.txt entry. * hrw4u: keep number spelling in every value context The AST parsed a NUMBER into a Python int, so a leading zero the emitter echoes back was lost: 007 became 7 in a header value, three bytes becoming one. number-spelling.input.txt witnesses all three value contexts; the digits survive in each. A plain int was a deliberate call, on the grounds that no corpus input wrote a leading zero. It did not survive a semantic pass built on the AST, which matches structurally over ValueExpr and has nowhere to put a naked int. * hrw4u: make the per-test sandbox fixture compile Its body wrote inbound.req.X-Foo inside TXN_START, where that field does not exist, so the input was rejected before the sandbox ran and the round-trip test had to name it as the one corpus input that does not compile. The fixture only ever asserted that a per-test sandbox.yaml is preferred over the shared one, and a section denial fires whatever the body is, so the body is now a rule TXN_START actually admits. That retires DOES_NOT_COMPILE: an input meant to be rejected is named .fail., and a corpus input that stops compiling should fail the test rather than opt out of it. (cherry picked from commit 2651038e6faa5312b190f49c515f600794de67cc) --- tools/hrw4u/src/ast_nodes.py | 49 +++++- tools/hrw4u/src/ast_visitor.py | 41 +++-- tools/hrw4u/tests/ast_unparse.py | 144 ++++++++++++++++ .../tests/data/ops/bool-spelling.input.txt | 7 + .../tests/data/ops/bool-spelling.output.txt | 5 + tools/hrw4u/tests/data/ops/exceptions.txt | 2 + .../tests/data/ops/number-spelling.input.txt | 7 + .../tests/data/ops/number-spelling.output.txt | 5 + .../procedures/local-bare-param.input.txt | 7 + .../procedures/local-bare-param.output.txt | 2 + .../denied-language-else-empty.error.txt | 1 + .../denied-language-else-empty.input.txt | 6 + .../denied-language-else-empty.sandbox.yaml | 6 + .../data/sandbox/per-test-sandbox.input.txt | 4 +- tools/hrw4u/tests/test_ast_roundtrip.py | 100 +++++++++++ tools/hrw4u/tests/test_ast_visitor.py | 156 ++++++++++++++---- 16 files changed, 487 insertions(+), 55 deletions(-) create mode 100644 tools/hrw4u/tests/ast_unparse.py create mode 100644 tools/hrw4u/tests/data/ops/bool-spelling.input.txt create mode 100644 tools/hrw4u/tests/data/ops/bool-spelling.output.txt create mode 100644 tools/hrw4u/tests/data/ops/number-spelling.input.txt create mode 100644 tools/hrw4u/tests/data/ops/number-spelling.output.txt create mode 100644 tools/hrw4u/tests/data/procedures/local-bare-param.input.txt create mode 100644 tools/hrw4u/tests/data/procedures/local-bare-param.output.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-else-empty.error.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-else-empty.input.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-else-empty.sandbox.yaml create mode 100644 tools/hrw4u/tests/test_ast_roundtrip.py diff --git a/tools/hrw4u/src/ast_nodes.py b/tools/hrw4u/src/ast_nodes.py index acf5bacccb3..2b8741e8ea8 100644 --- a/tools/hrw4u/src/ast_nodes.py +++ b/tools/hrw4u/src/ast_nodes.py @@ -25,16 +25,22 @@ "IdentValue", "IPValue", "ParamRef", + "BoolValue", + "NumberValue", "RegexValue", + "SetValue", + "IpRangeValue", "ValueExpr", "Node", "Target", "Assignment", "FunctionCall", "Break", + "Comment", "Comparison", "LogicalOp", "NotOp", + "Group", "BoolLiteral", "IdentCondition", "ElifBranch", @@ -72,12 +78,32 @@ class ParamRef: raw: str +@dataclass(frozen=True, kw_only=True) +class BoolValue: + raw: str # source spelling, e.g. "TRUE", "true", "TRue" + + +@dataclass(frozen=True, kw_only=True) +class NumberValue: + raw: str # source spelling: header_rewrite echoes it, so 007 is three bytes and not 7 + + @dataclass(frozen=True, kw_only=True) class RegexValue: raw: str -ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int, bool, tuple[IPValue, ...]] +@dataclass(frozen=True, kw_only=True) +class SetValue: + raw: str # bracket-stripped source text, quoting preserved + + +@dataclass(frozen=True, kw_only=True) +class IpRangeValue: + raw: str # verbatim source text + + +ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, BoolValue, NumberValue, IpRangeValue] @dataclass(frozen=True, kw_only=True) @@ -119,11 +145,16 @@ class Break(Node): pass +@dataclass(frozen=True, kw_only=True) +class Comment(Node): + text: str + + @dataclass(frozen=True, kw_only=True) class Comparison(Node): left: IdentValue | FunctionCall operator: str # "==", "!=", ">", "<", "~", "!~", "in", "!in" - right: ValueExpr | RegexValue | tuple[ValueExpr, ...] + right: ValueExpr | RegexValue | SetValue | IpRangeValue modifiers: tuple[str, ...] @@ -139,6 +170,11 @@ class NotOp(Node): operand: ConditionExpr +@dataclass(frozen=True, kw_only=True) +class Group(Node): + inner: ConditionExpr + + @dataclass(frozen=True, kw_only=True) class BoolLiteral(Node): value: bool @@ -161,6 +197,7 @@ class IfBlock(Node): body: tuple[BodyNode, ...] elif_branches: tuple[ElifBranch, ...] else_body: tuple[BodyNode, ...] + has_else: bool # an empty else body is not the same as no else clause @dataclass(frozen=True, kw_only=True) @@ -185,7 +222,7 @@ class VarDecl(Node): @dataclass(frozen=True, kw_only=True) class VarSection(Node): scope: str - declarations: tuple[VarDecl, ...] + declarations: tuple[VarDecl | Comment, ...] @dataclass(frozen=True, kw_only=True) @@ -206,6 +243,6 @@ class HRW4UAST: # Type aliases: must follow all class definitions (evaluated at runtime). -ConditionExpr = Union[Comparison, LogicalOp, NotOp, BoolLiteral, IdentCondition, FunctionCall] -BodyNode = Union[Assignment, FunctionCall, IfBlock, Break] -TopLevelNode = Union[UseDirective, VarSection, ProcedureDecl, Section] +ConditionExpr = Union[Comparison, LogicalOp, NotOp, Group, BoolLiteral, IdentCondition, FunctionCall] +BodyNode = Union[Assignment, FunctionCall, IfBlock, Break, Comment] +TopLevelNode = Union[UseDirective, VarSection, ProcedureDecl, Section, Comment] diff --git a/tools/hrw4u/src/ast_visitor.py b/tools/hrw4u/src/ast_visitor.py index 4a66ec0a710..df376cedb78 100644 --- a/tools/hrw4u/src/ast_visitor.py +++ b/tools/hrw4u/src/ast_visitor.py @@ -29,6 +29,9 @@ class ASTVisitor(hrw4uVisitor): # method has an explicit return type and full control over how # child results are assembled into parent AST nodes. + def _visit_comment(self, ctx) -> Comment: + return Comment(text=ctx.COMMENT().getText(), line=ctx.start.line) + def visitProgram(self, ctx) -> HRW4UAST: items = [] for item in ctx.programItem(): @@ -39,7 +42,7 @@ def visitProgram(self, ctx) -> HRW4UAST: elif item.section() is not None: items.append(self._visit_section(item.section())) elif item.commentLine() is not None: - pass + items.append(self._visit_comment(item.commentLine())) else: raise ValueError(f"Unhandled programItem alternative at line {item.start.line}") return HRW4UAST(body=tuple(items)) @@ -75,7 +78,7 @@ def _visit_var_section(self, ctx, scope) -> VarSection: if var_item.variableDecl() is not None: decls.append(self._visit_var_decl(var_item.variableDecl())) elif var_item.commentLine() is not None: - pass + decls.append(self._visit_comment(var_item.commentLine())) else: raise ValueError(f"Unhandled variablesItem alternative at line {var_item.start.line}") return VarSection(scope=scope, declarations=tuple(decls), line=ctx.start.line) @@ -93,7 +96,7 @@ def _visit_body(self, items) -> list[BodyNode]: elif item.conditional() is not None: result.append(self._visit_conditional(item.conditional())) elif item.commentLine() is not None: - pass + result.append(self._visit_comment(item.commentLine())) else: raise ValueError(f"Unhandled body item alternative at line {item.start.line}") return result @@ -125,19 +128,19 @@ def _visit_function_call(self, ctx) -> FunctionCall: def _extract_value(self, ctx) -> ValueExpr: if ctx.number is not None: - return int(ctx.number.text) + return NumberValue(raw=ctx.number.text) if ctx.str_ is not None: return LiteralStringValue(raw=ctx.str_.text[1:-1]) if ctx.TRUE(): - return True + return BoolValue(raw=ctx.TRUE().getText()) if ctx.FALSE(): - return False + return BoolValue(raw=ctx.FALSE().getText()) if ctx.ident is not None: return IdentValue(raw=ctx.ident.text) if ctx.ip(): return IPValue(raw=ctx.ip().getText()) if ctx.iprange(): - return tuple(IPValue(raw=ip.getText()) for ip in ctx.iprange().ip()) + return IpRangeValue(raw=ctx.iprange().getText()) if ctx.paramRef(): return ParamRef(raw=ctx.paramRef().IDENT().getText()) raise ValueError(f"Unhandled value alternative at line {ctx.start.line}") @@ -155,13 +158,17 @@ def _visit_conditional(self, ctx) -> IfBlock: elif_body = tuple(self._visit_body(elif_block.blockItem())) if elif_block else () elif_branches.append(ElifBranch(condition=elif_cond, body=elif_body, line=elif_ctx.start.line)) - else_body = () - if ctx.elseClause(): - else_block = ctx.elseClause().block() - if else_block: - else_body = tuple(self._visit_body(else_block.blockItem())) + else_clause = ctx.elseClause() + else_block = else_clause.block() if else_clause else None + else_body = tuple(self._visit_body(else_block.blockItem())) if else_block else () - return IfBlock(condition=condition, body=body, elif_branches=tuple(elif_branches), else_body=else_body, line=ctx.start.line) + return IfBlock( + condition=condition, + body=body, + elif_branches=tuple(elif_branches), + else_body=else_body, + has_else=else_clause is not None, + line=ctx.start.line) def _visit_condition(self, ctx) -> ConditionExpr: return self._visit_expression(ctx.expression()) @@ -184,7 +191,7 @@ def _visit_factor(self, ctx) -> ConditionExpr: if ctx.getChildCount() == 2 and ctx.getChild(0).getText() == "!": return NotOp(operand=self._visit_factor(ctx.factor()), line=ctx.start.line) if ctx.LPAREN(): - return self._visit_expression(ctx.expression()) + return Group(inner=self._visit_expression(ctx.expression()), line=ctx.start.line) if ctx.functionCall(): return self._visit_function_call(ctx.functionCall()) if ctx.comparison(): @@ -231,14 +238,14 @@ def _detect_comparison_operator(self, ctx) -> str: return "in" raise ValueError(f"Unhandled comparison operator at line {ctx.start.line}") - def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | tuple[ValueExpr, ...]: + def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | SetValue | IpRangeValue: if operator in ("~", "!~"): return RegexValue(raw=ctx.regex().getText()[1:-1]) if operator in ("in", "!in"): if ctx.set_(): - return tuple(self._extract_value(v) for v in ctx.set_().value()) + return SetValue(raw=ctx.set_().getText()[1:-1]) if ctx.iprange(): - return tuple(IPValue(raw=ip.getText()) for ip in ctx.iprange().ip()) + return IpRangeValue(raw=ctx.iprange().getText()) if ctx.value(): return self._extract_value(ctx.value()) raise ValueError(f"Unhandled comparison RHS at line {ctx.start.line}") diff --git a/tools/hrw4u/tests/ast_unparse.py b/tools/hrw4u/tests/ast_unparse.py new file mode 100644 index 00000000000..11cd17fe6ae --- /dev/null +++ b/tools/hrw4u/tests/ast_unparse.py @@ -0,0 +1,144 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Render an AST back to hrw4u source. Test-only; see test_ast_roundtrip.py for why. + +Whitespace is not reproduced: the emitter derives its own indentation and never reads the +source's. +""" + +from __future__ import annotations + +from hrw4u.ast_nodes import * + +INDENT = " " + + +def unparse(ast: HRW4UAST) -> str: + return "\n".join(_top_level(item) for item in ast.body) + "\n" + + +def _top_level(node: TopLevelNode) -> str: + match node: + case Comment(): + return node.text + case UseDirective(): + return f"use {node.spec}" + case ProcedureDecl(): + params = ", ".join(_proc_param(p) for p in node.params) + return _braced(f"procedure {node.name}({params})", [_body_item(b, 1) for b in node.body]) + case VarSection(): + keyword = "SESSION_VARS" if node.scope == "session" else "VARS" + return _braced(keyword, [_var_item(d) for d in node.declarations]) + case Section(): + return _braced(node.type, [_body_item(b, 1) for b in node.body]) + raise ValueError(f"unparse: unhandled top-level node {type(node).__name__}") + + +def _braced(header: str, lines: list[str]) -> str: + return "\n".join([f"{header} {{", *(f"{INDENT}{line}" for line in lines), "}"]) + + +def _proc_param(p: ProcParam) -> str: + return f"${p.name}" if p.default is None else f"${p.name}={_value(p.default)}" + + +def _var_item(node: VarDecl | Comment) -> str: + if isinstance(node, Comment): + return node.text + slot = "" if node.slot is None else f" @{node.slot}" + return f"{node.name}: {node.type_name}{slot};" + + +def _body_item(node: BodyNode, depth: int) -> str: + match node: + case Comment(): + return node.text + case Break(): + return "break;" + case FunctionCall(): + return f"{_call(node)};" + case Assignment(): + return f"{_target(node.target)} {node.operator} {_value(node.value)};" + case IfBlock(): + return _if_block(node, depth) + raise ValueError(f"unparse: unhandled body node {type(node).__name__}") + + +def _if_block(node: IfBlock, depth: int) -> str: + pad = INDENT * depth + lines = [f"if {_condition(node.condition)} {{"] + lines += [f"{INDENT}{line}" for line in _nested(node.body, depth)] + for arm in node.elif_branches: + lines.append(f"}} elif {_condition(arm.condition)} {{") + lines += [f"{INDENT}{line}" for line in _nested(arm.body, depth)] + if node.has_else: + lines.append("} else {") + lines += [f"{INDENT}{line}" for line in _nested(node.else_body, depth)] + lines.append("}") + return f"\n{pad}".join(lines) + + +def _nested(body: tuple[BodyNode, ...], depth: int) -> list[str]: + return [line for item in body for line in _body_item(item, depth + 1).splitlines()] + + +def _target(t: Target) -> str: + return t.field if t.namespace is None else f"{t.namespace}.{t.field}" + + +def _call(node: FunctionCall) -> str: + return f"{node.name}({', '.join(_value(a) for a in node.args)})" + + +def _condition(node: ConditionExpr) -> str: + match node: + case Group(): + return f"({_condition(node.inner)})" + case LogicalOp(): + return f"{_condition(node.left)} {node.operator} {_condition(node.right)}" + case NotOp(): + return f"!{_condition(node.operand)}" + case BoolLiteral(): + return "true" if node.value else "false" + case IdentCondition(): + return node.name + case FunctionCall(): + return _call(node) + case Comparison(): + return _comparison(node) + raise ValueError(f"unparse: unhandled condition node {type(node).__name__}") + + +def _comparison(node: Comparison) -> str: + left = node.left.raw if isinstance(node.left, IdentValue) else _call(node.left) + mods = f" with {', '.join(node.modifiers)}" if node.modifiers else "" + return f"{left} {node.operator} {_value(node.right)}{mods}" + + +def _value(v: ValueExpr | RegexValue | SetValue) -> str: + match v: + case LiteralStringValue(): + return f'"{v.raw}"' + case NumberValue() | BoolValue() | IdentValue() | IPValue() | IpRangeValue(): + return v.raw + case ParamRef(): + return f"${v.raw}" + case RegexValue(): + return f"/{v.raw}/" + case SetValue(): + return f"[{v.raw}]" + raise ValueError(f"unparse: unhandled value {type(v).__name__}") diff --git a/tools/hrw4u/tests/data/ops/bool-spelling.input.txt b/tools/hrw4u/tests/data/ops/bool-spelling.input.txt new file mode 100644 index 00000000000..750713155e8 --- /dev/null +++ b/tools/hrw4u/tests/data/ops/bool-spelling.input.txt @@ -0,0 +1,7 @@ +# The emitter echoes a bool's spelling, so the AST cannot normalize one anywhere: +# an assignment RHS is not the only value context that reaches header_rewrite. +REMAP { + if inbound.req.X-Debug == TRUE { + set-config("proxy.config.http.cache.http", FALSE); + } +} diff --git a/tools/hrw4u/tests/data/ops/bool-spelling.output.txt b/tools/hrw4u/tests/data/ops/bool-spelling.output.txt new file mode 100644 index 00000000000..25575881097 --- /dev/null +++ b/tools/hrw4u/tests/data/ops/bool-spelling.output.txt @@ -0,0 +1,5 @@ +# The emitter echoes a bool's spelling, so the AST cannot normalize one anywhere: +# an assignment RHS is not the only value context that reaches header_rewrite. +cond %{REMAP_PSEUDO_HOOK} [AND] +cond %{CLIENT-HEADER:X-Debug} =TRUE + set-config "proxy.config.http.cache.http" FALSE diff --git a/tools/hrw4u/tests/data/ops/exceptions.txt b/tools/hrw4u/tests/data/ops/exceptions.txt index d954a6ae38a..aafd6d20220 100644 --- a/tools/hrw4u/tests/data/ops/exceptions.txt +++ b/tools/hrw4u/tests/data/ops/exceptions.txt @@ -7,3 +7,5 @@ qsa.input: u4wrh header_value_context.input: u4wrh # HTTP-CNTL valid bools can not reverse back to the original input http_cntl_valid_bools.input: hrw4u +# The reverse normalizes a bool argument's spelling (FALSE -> false) +bool-spelling.input: hrw4u diff --git a/tools/hrw4u/tests/data/ops/number-spelling.input.txt b/tools/hrw4u/tests/data/ops/number-spelling.input.txt new file mode 100644 index 00000000000..05680bf58a4 --- /dev/null +++ b/tools/hrw4u/tests/data/ops/number-spelling.input.txt @@ -0,0 +1,7 @@ +# The emitter echoes a number's digits, so the AST cannot normalize one anywhere: +# 007 and 7 are different bytes once a number reaches a header value. +REMAP { + if random(0100) > 007 { + inbound.req.X-Count = 007; + } +} diff --git a/tools/hrw4u/tests/data/ops/number-spelling.output.txt b/tools/hrw4u/tests/data/ops/number-spelling.output.txt new file mode 100644 index 00000000000..6988ba47e22 --- /dev/null +++ b/tools/hrw4u/tests/data/ops/number-spelling.output.txt @@ -0,0 +1,5 @@ +# The emitter echoes a number's digits, so the AST cannot normalize one anywhere: +# 007 and 7 are different bytes once a number reaches a header value. +cond %{REMAP_PSEUDO_HOOK} [AND] +cond %{RANDOM:0100} >007 + set-header X-Count 007 diff --git a/tools/hrw4u/tests/data/procedures/local-bare-param.input.txt b/tools/hrw4u/tests/data/procedures/local-bare-param.input.txt new file mode 100644 index 00000000000..1cbee7941d1 --- /dev/null +++ b/tools/hrw4u/tests/data/procedures/local-bare-param.input.txt @@ -0,0 +1,7 @@ +procedure local::tag($name) { + inbound.req.X-Tag = $name; +} + +REMAP { + local::tag("hello"); +} diff --git a/tools/hrw4u/tests/data/procedures/local-bare-param.output.txt b/tools/hrw4u/tests/data/procedures/local-bare-param.output.txt new file mode 100644 index 00000000000..82acc54b246 --- /dev/null +++ b/tools/hrw4u/tests/data/procedures/local-bare-param.output.txt @@ -0,0 +1,2 @@ +cond %{REMAP_PSEUDO_HOOK} [AND] + set-header X-Tag hello diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.error.txt b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.error.txt new file mode 100644 index 00000000000..4aa2d20b1d5 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.error.txt @@ -0,0 +1 @@ +'else' is denied by sandbox policy (language) diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.input.txt b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.input.txt new file mode 100644 index 00000000000..dcaf3a6d84d --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.input.txt @@ -0,0 +1,6 @@ +REMAP { + if inbound.req.X-Foo == "a" { + inbound.req.X-Result = "yes"; + } else { + } +} diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.sandbox.yaml b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.sandbox.yaml new file mode 100644 index 00000000000..58c2bde66b0 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-else-empty.sandbox.yaml @@ -0,0 +1,6 @@ +sandbox: + message: "Feature denied by sandbox policy. Contact platform team." + + deny: + language: + - else diff --git a/tools/hrw4u/tests/data/sandbox/per-test-sandbox.input.txt b/tools/hrw4u/tests/data/sandbox/per-test-sandbox.input.txt index bdfdf647a71..5ccf2f3bdce 100644 --- a/tools/hrw4u/tests/data/sandbox/per-test-sandbox.input.txt +++ b/tools/hrw4u/tests/data/sandbox/per-test-sandbox.input.txt @@ -1,3 +1,5 @@ TXN_START { - inbound.req.X-Foo = "test"; + if inbound.ip in {10.0.0.0/8} { + counter("txn.internal"); + } } diff --git a/tools/hrw4u/tests/test_ast_roundtrip.py b/tools/hrw4u/tests/test_ast_roundtrip.py new file mode 100644 index 00000000000..a0e8c411b34 --- /dev/null +++ b/tools/hrw4u/tests/test_ast_roundtrip.py @@ -0,0 +1,100 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""The AST must carry everything the emitter reads out of the source. + +Hand-written cases only catch losses someone thought of; an empty `else { }`, a dropped +comment and a `TRUE`/`true` spelling all shipped because nobody did. So instead of naming +distinctions, run the whole corpus through the AST and back and require the compiled +config to be unchanged: whatever the AST drops, the config loses too. + + corpus .hrw4u --parse--> AST --unparse--> regenerated .hrw4u + | | + emit emit + | | + v v + config <----------- must match ------------> config + +The comparison is the compiled config, never the regenerated source text. An AST holds no +whitespace, indentation or blank lines, and the emitter reads none of them -- it even +indents a preserved comment by nesting depth rather than by the column it came from. So +demanding byte-identical source would fail on almost every input for reasons that change +no output, and satisfying it would mean turning the AST back into a CST. + +A companion test asserts the corpus reaches every grammar rule, so a rule with no fixture +is reported rather than silently unguarded. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from antlr4 import ParserRuleContext + +import ast_unparse +import utils +from hrw4u.ast_visitor import ASTVisitor +from hrw4u.hrw4uParser import hrw4uParser +from hrw4u.visitor import HRW4UVisitor + +CORPUS = Path("tests/data") + + +def _case_id(input_file: Path) -> str: + return f"{input_file.parent.name}/{input_file.name.removesuffix('.input.txt')}" + + +def _cases() -> list[pytest.param]: + files = (f for f in sorted(CORPUS.glob("*/*.input.txt")) if ".fail." not in f.name) + return [pytest.param(f, id=_case_id(f)) for f in files] + + +def _compile(text: str, input_file: Path) -> list[str]: + _, tree = utils.parse_input_text(text) + visitor = HRW4UVisitor(filename=str(input_file), proc_search_paths=[input_file.parent / "procs"]) + return visitor.visit(tree) + + +@pytest.mark.parametrize("input_file", _cases()) +def test_emitted_config_survives_a_round_trip_through_the_ast(input_file: Path) -> None: + source = input_file.read_text() + _, tree = utils.parse_input_text(source) + regenerated = ast_unparse.unparse(ASTVisitor().visit(tree)) + + expected = _compile(source, input_file) + # An empty config would pass no matter what the AST drops. + assert expected, f"{input_file} compiles to nothing; it cannot witness a round trip" + assert _compile(regenerated, input_file) == expected, ( + f"{input_file}: the AST lost something the emitter reads.\n" + f"--- regenerated hrw4u ---\n{regenerated}") + + +def test_the_corpus_reaches_every_grammar_rule() -> None: + reached: set[str] = set() + + def walk(ctx) -> None: + if isinstance(ctx, ParserRuleContext): + reached.add(hrw4uParser.ruleNames[ctx.getRuleIndex()]) + for child in ctx.getChildren(): + walk(child) + + for param in _cases(): + _, tree = utils.parse_input_text(param.values[0].read_text()) + walk(tree) + + missing = set(hrw4uParser.ruleNames) - reached + assert not missing, f"no corpus input exercises: {', '.join(sorted(missing))}" diff --git a/tools/hrw4u/tests/test_ast_visitor.py b/tools/hrw4u/tests/test_ast_visitor.py index ec919d1f060..2df26b5b5da 100644 --- a/tools/hrw4u/tests/test_ast_visitor.py +++ b/tools/hrw4u/tests/test_ast_visitor.py @@ -15,6 +15,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import pytest + from hrw4u.ast_nodes import * from utils import parse_input_text from hrw4u.ast_visitor import ASTVisitor @@ -39,12 +41,26 @@ def test_bool_value(self): ast = _build('SEND_RESPONSE {\n http.cntl.TXN_DEBUG = true;\n}') a = ast.body[0].body[0] assert isinstance(a, Assignment) - assert a.value is True + assert a.value == BoolValue(raw="true") + + def test_bool_assignment_keeps_source_spelling(self): + """The emitter echoes the spelling back, so the RHS cannot normalize to a plain bool.""" + upper = _build('SEND_RESPONSE {\n http.cntl.TXN_DEBUG = TRUE;\n}').body[0].body[0] + lower = _build('SEND_RESPONSE {\n http.cntl.TXN_DEBUG = true;\n}').body[0].body[0] + assert upper.value == BoolValue(raw="TRUE") + assert lower.value == BoolValue(raw="true") + + def test_bool_keeps_source_spelling_outside_an_assignment(self): + """A procedure default is bound raw into its use site, so it echoes its spelling too.""" + src = 'procedure local::p($on=true, $off=FALSE) {\n set-debug();\n}\nREMAP {\n set-debug();\n}' + pd = _build(src).body[0] + assert pd.params[0].default == BoolValue(raw="true") + assert pd.params[1].default == BoolValue(raw="FALSE") def test_int_value(self): ast = _build('REMAP {\n http.cntl.INTERCEPT_RETRY = 1;\n}') a = ast.body[0].body[0] - assert a.value == 1 + assert a.value == NumberValue(raw="1") def test_plus_equals(self): ast = _build('REMAP {\n inbound.req.X-Foo += "extra";\n}') @@ -57,6 +73,16 @@ def test_ip_value(self): assert isinstance(a, Assignment) assert a.value == IPValue(raw="10.0.0.1") + def test_ident_value(self): + src = 'VARS {\n a: bool;\n b: bool;\n}\nREMAP {\n b = a;\n}' + a = _build(src).body[1].body[0] + assert isinstance(a, Assignment) + assert a.value == IdentValue(raw="a") + + def test_iprange_value(self): + a = _build('REMAP {\n inbound.req.X = {1.2.3.4, 5.6.7.8};\n}').body[0].body[0] + assert a.value == IpRangeValue(raw="{1.2.3.4,5.6.7.8}") + def test_param_ref_value(self): src = 'procedure local::stamp($tag) {\n inbound.req.X-Stamp = $tag;\n}\nREMAP {\n set-debug();\n}' ast = _build(src) @@ -95,15 +121,15 @@ def test_break(self): class TestSections: - def test_comments_in_section_body_skipped(self): + def test_comments_in_section_body_preserved(self): src = 'REMAP {\n # a comment\n set-debug();\n # another comment\n}' ast = _build(src) - assert len(ast.body[0].body) == 1 + assert len(ast.body[0].body) == 3 - def test_comments_in_block_skipped(self): + def test_comments_in_block_preserved(self): src = 'REMAP {\n if true {\n # comment\n set-debug();\n }\n}' ast = _build(src) - assert len(ast.body[0].body[0].body) == 1 + assert len(ast.body[0].body[0].body) == 2 def test_section_type(self): ast = _build('REMAP {\n set-debug();\n}') @@ -138,12 +164,12 @@ def test_item_ordering(self): class TestVarSections: - def test_comments_in_var_section_skipped(self): + def test_comments_in_var_section_preserved(self): src = 'VARS {\n # comment\n x: bool;\n # another\n y: int;\n}\nREMAP {\n set-debug();\n}' ast = _build(src) vs = ast.body[0] assert isinstance(vs, VarSection) - assert len(vs.declarations) == 2 + assert len(vs.declarations) == 4 def test_txn_scope(self): src = 'VARS {\n flag: bool;\n}\nREMAP {\n set-debug();\n}' @@ -200,7 +226,7 @@ def test_default_param(self): pd = ast.body[0] assert isinstance(pd, ProcedureDecl) assert pd.params[0].name == "ttl" - assert pd.params[0].default == 300 + assert pd.params[0].default == NumberValue(raw="300") def test_body(self): src = ('procedure local::multi() {\n inbound.req.X = "a";\n' @@ -237,7 +263,7 @@ def test_in_set(self): cond = self._first_condition('REMAP {\n if inbound.url.path in ["a", "b"] {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (LiteralStringValue(raw="a"), LiteralStringValue(raw="b")) + assert cond.right == SetValue(raw='"a","b"') def test_not_in_set(self): cond = self._first_condition('REMAP {\n if inbound.url.path !in ["a"] {\n set-debug();\n }\n}') @@ -248,7 +274,7 @@ def test_in_iprange(self): cond = self._first_condition('REMAP {\n if inbound.ip in {10.0.0.0/8} {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (IPValue(raw="10.0.0.0/8"),) + assert cond.right == IpRangeValue(raw="{10.0.0.0/8}") def test_modifiers(self): cond = self._first_condition('REMAP {\n if inbound.req.X-Foo == "bar" with NOCASE {\n set-debug();\n }\n}') @@ -265,7 +291,7 @@ def test_function_call_comparable(self): assert isinstance(cond, Comparison) assert isinstance(cond.left, FunctionCall) assert cond.left.name == "url" - assert cond.left.args == (True,) + assert cond.left.args == (BoolValue(raw="true"),) def test_bool_literal_true(self): cond = self._first_condition('REMAP {\n if true {\n set-debug();\n }\n}') @@ -312,13 +338,19 @@ def test_greater_than_comparison(self): cond = self._first_condition('REMAP {\n if inbound.req.Content-Length > 1000 {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == ">" - assert cond.right == 1000 + assert cond.right == NumberValue(raw="1000") def test_less_than_comparison(self): cond = self._first_condition('REMAP {\n if inbound.req.Content-Length < 500 {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == "<" - assert cond.right == 500 + assert cond.right == NumberValue(raw="500") + + def test_comparison_rhs_keeps_bool_spelling(self): + """`== TRUE` emits `=TRUE`, so normalizing the RHS would change the config.""" + cond = self._first_condition('REMAP {\n if inbound.req.X-Debug == TRUE {\n set-debug();\n }\n}') + assert isinstance(cond, Comparison) + assert cond.right == BoolValue(raw="TRUE") def test_neq_comparison(self): cond = self._first_condition('REMAP {\n if inbound.req.X-Foo != "bar" {\n set-debug();\n }\n}') @@ -328,9 +360,25 @@ def test_neq_comparison(self): def test_parenthesized_condition(self): cond = self._first_condition('REMAP {\n if (inbound.req.X-Foo == "bar") {\n set-debug();\n }\n}') - assert isinstance(cond, Comparison) - assert cond.operator == "==" - assert cond.right == LiteralStringValue(raw="bar") + assert isinstance(cond, Group) + assert isinstance(cond.inner, Comparison) + assert cond.inner.operator == "==" + assert cond.inner.right == LiteralStringValue(raw="bar") + + def test_parens_are_kept(self): + """A parenthesized factor becomes cond %{GROUP}.""" + grouped = self._first_condition('REMAP {\n if (true) {\n set-debug();\n }\n}') + bare = self._first_condition('REMAP {\n if true {\n set-debug();\n }\n}') + assert isinstance(grouped, Group) + assert isinstance(grouped.inner, BoolLiteral) + assert isinstance(bare, BoolLiteral) + + def test_set_and_iprange_are_distinguishable(self): + """`in [1.2.3.4]` emits (1.2.3.4) but `in {1.2.3.4}` emits {1.2.3.4}.""" + as_set = self._first_condition('REMAP {\n if inbound.ip in [1.2.3.4] {\n set-debug();\n }\n}') + as_range = self._first_condition('REMAP {\n if inbound.ip in {1.2.3.4} {\n set-debug();\n }\n}') + assert as_set.right == SetValue(raw="1.2.3.4") + assert as_range.right == IpRangeValue(raw="{1.2.3.4}") def test_and_binds_tighter_than_or(self): # a || b && c should parse as a || (b && c) @@ -370,9 +418,9 @@ def test_not_comparison_with_or(self): assert isinstance(cond, LogicalOp) assert cond.operator == "||" assert isinstance(cond.left, NotOp) - assert isinstance(cond.left.operand, Comparison) - assert cond.left.operand.left == IdentValue(raw="inbound.req.X-A") - assert cond.left.operand.right == LiteralStringValue(raw="x") + assert isinstance(cond.left.operand, Group) + assert cond.left.operand.inner.left == IdentValue(raw="inbound.req.X-A") + assert cond.left.operand.inner.right == LiteralStringValue(raw="x") assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-B") @@ -397,10 +445,10 @@ def test_parens_override_precedence(self): ' set-debug();\n }\n}') assert isinstance(cond, LogicalOp) assert cond.operator == "&&" - assert isinstance(cond.left, LogicalOp) - assert cond.left.operator == "||" - assert cond.left.left.left == IdentValue(raw="inbound.req.X-A") - assert cond.left.right.left == IdentValue(raw="inbound.req.X-B") + assert isinstance(cond.left, Group) + assert cond.left.inner.operator == "||" + assert cond.left.inner.left.left == IdentValue(raw="inbound.req.X-A") + assert cond.left.inner.right.left == IdentValue(raw="inbound.req.X-B") assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-C") @@ -413,8 +461,8 @@ def test_nested_parens_with_not(self): assert isinstance(cond, LogicalOp) assert cond.operator == "&&" assert isinstance(cond.left, NotOp) - assert isinstance(cond.left.operand, LogicalOp) - assert cond.left.operand.operator == "||" + assert isinstance(cond.left.operand, Group) + assert cond.left.operand.inner.operator == "||" assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-C") @@ -433,8 +481,32 @@ def test_if_else(self): src = 'REMAP {\n if true {\n inbound.req.X = "a";\n } else {\n inbound.req.X = "b";\n }\n}' ast = _build(src) ib = ast.body[0].body[0] + assert ib.has_else is True assert len(ib.else_body) == 1 + def test_empty_else_is_distinguishable_from_no_else(self): + """Gating on else_body alone lets a sandbox policy denying 'else' be evaded.""" + no_else = _build('REMAP {\n if true {\n inbound.req.X = "y";\n }\n}').body[0].body[0] + empty_else = _build('REMAP {\n if true {\n inbound.req.X = "y";\n } else {\n }\n}').body[0].body[0] + assert no_else.has_else is False + assert empty_else.has_else is True + assert empty_else.else_body == () + + def test_empty_else_after_elif_sets_has_else(self): + src = ( + 'REMAP {\n if inbound.req.X == "a" {\n set-debug();\n' + ' } elif inbound.req.X == "b" {\n set-debug();\n' + ' } else {\n }\n}') + ib = _build(src).body[0].body[0] + assert len(ib.elif_branches) == 1 + assert ib.has_else is True + assert ib.else_body == () + + def test_has_else_is_required(self): + """A default would silently mean "no else" at any node-rebuilding site.""" + with pytest.raises(TypeError): + IfBlock(condition=BoolLiteral(value=True, line=1), body=(), elif_branches=(), else_body=(), line=1) + def test_if_elif_else(self): src = ( 'SEND_RESPONSE {\n if inbound.url.path == "foo" {\n' @@ -686,8 +758,8 @@ def test_http_cntl_booleans(self): }''' ast = _build(src) body = ast.body[0].body - assert body[0].value is True - assert body[1].value is False + assert body[0].value == BoolValue(raw="true") + assert body[1].value == BoolValue(raw="FALSE") def test_ip_range_condition(self): """Validates IP range handling from tests/data/conds/ip.input.txt.""" @@ -700,7 +772,7 @@ def test_ip_range_condition(self): cond = ast.body[0].body[0].condition assert isinstance(cond, Comparison) assert cond.operator == "in" - assert len(cond.right) == 2 + assert cond.right == IpRangeValue(raw="{192.168.0.0/16,10.0.0.0/8}") def test_set_membership_with_modifier(self): """From tests/data/conds/in-sets.input.txt.""" @@ -713,7 +785,7 @@ def test_set_membership_with_modifier(self): cond = ast.body[0].body[0].condition assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (LiteralStringValue(raw="php"), LiteralStringValue(raw="php3"), LiteralStringValue(raw="php4")) + assert cond.right == SetValue(raw='"php","php3","php4"') assert cond.modifiers == ("EXT",) def test_debug_pattern_for_lint_rules(self): @@ -733,8 +805,30 @@ def test_debug_pattern_for_lint_rules(self): # TXN_DEBUG assignment with True assert isinstance(body[1], Assignment) assert body[1].target == Target.from_dotted("http.cntl.TXN_DEBUG") - assert body[1].value is True + assert body[1].value == BoolValue(raw="true") # Regular assignment (not flagged) assert isinstance(body[2], Assignment) assert body[2].target.namespace == "inbound.req" + + +class TestComments: + + def test_top_level_comment_preserved(self): + ast = _build('# hello\nREMAP {\n set-debug();\n}') + assert ast.body[0] == Comment(text="# hello", line=1) + + def test_comment_in_section_body_keeps_position(self): + body = _build('REMAP {\n # first\n set-debug();\n}').body[0].body + assert isinstance(body[0], Comment) + assert body[0].text == "# first" + assert isinstance(body[1], FunctionCall) + + def test_comment_in_block(self): + ast = _build('REMAP {\n if inbound.status > 399 {\n # why\n set-debug();\n }\n}') + assert isinstance(ast.body[0].body[0].body[0], Comment) + + def test_comment_in_vars_section(self): + decls = _build('VARS {\n # a counter\n hits: int8;\n}').body[0].declarations + assert isinstance(decls[0], Comment) + assert isinstance(decls[1], VarDecl) From 5a02191d6aaaae3019db0d3b96cbfe37d5be8591 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 22 Sep 2026 21:00:45 +0900 Subject: [PATCH 5/8] hrw4u: enforce sandbox NOT and in on their implicit spellings (#13675) * hrw4u: enforce sandbox NOT and in on their implicit spellings `modifiers: [NOT]` only caught an explicit `with NOT`, and `language: [in]` only caught the `[...]` value form, so `!expr`, `!=`, `!~`, `!in` and `in {10.0.0.0/8}` all compiled unchecked. AND and OR were already checked at `&&` and `||`; negation and IP-range membership are now consistent with that. The check sits at the two sites where the source introduces negation, not at `_make_condition`, whose `negate` argument is also true for the `[NOT]` the compiler synthesises for a bare header test. Denying `NOT` must not reject `if inbound.req.X-Foo`; allowed-implicit-not pins that. `modifiers` was undocumented, so the sandbox section gains it. * Address review: name the tag the compiler actually emits The modifiers example said `%{HEADER:X-Foo}`, but `inbound.req.X-Foo` lowers to CLIENT-HEADER, as allowed-implicit-not's golden output shows. Kept the literal on one line while here. * Address review: cover the !~ spelling of NOT NOT_TILDE is its own lexer token; without a case for it, dropping it from the negate tuple still passed. (cherry picked from commit ca133303832de2cb0c1d5b2ea38c9d2cc5d53820) --- doc/admin-guide/configuration/hrw4u.en.rst | 18 +++++++++++++++++- tools/hrw4u/src/visitor.py | 7 +++++++ .../data/sandbox/allowed-implicit-not.ast.txt | 1 + .../sandbox/allowed-implicit-not.input.txt | 5 +++++ .../sandbox/allowed-implicit-not.output.txt | 3 +++ .../sandbox/allowed-implicit-not.sandbox.yaml | 6 ++++++ .../sandbox/denied-language-in-iprange.ast.txt | 1 + .../denied-language-in-iprange.error.txt | 3 +++ .../denied-language-in-iprange.input.txt | 9 +++++++++ .../denied-language-in-iprange.sandbox.yaml | 6 ++++++ .../data/sandbox/denied-modifier-not.ast.txt | 1 + .../data/sandbox/denied-modifier-not.error.txt | 3 +++ .../data/sandbox/denied-modifier-not.input.txt | 17 +++++++++++++++++ .../sandbox/denied-modifier-not.sandbox.yaml | 6 ++++++ 14 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tools/hrw4u/tests/data/sandbox/allowed-implicit-not.ast.txt create mode 100644 tools/hrw4u/tests/data/sandbox/allowed-implicit-not.input.txt create mode 100644 tools/hrw4u/tests/data/sandbox/allowed-implicit-not.output.txt create mode 100644 tools/hrw4u/tests/data/sandbox/allowed-implicit-not.sandbox.yaml create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.ast.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.error.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.input.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.sandbox.yaml create mode 100644 tools/hrw4u/tests/data/sandbox/denied-modifier-not.ast.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-modifier-not.error.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-modifier-not.input.txt create mode 100644 tools/hrw4u/tests/data/sandbox/denied-modifier-not.sandbox.yaml diff --git a/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst index e387d0f2fb5..b6953df6415 100644 --- a/doc/admin-guide/configuration/hrw4u.en.rst +++ b/doc/admin-guide/configuration/hrw4u.en.rst @@ -718,6 +718,7 @@ Schema for editor validation and autocomplete is provided at conditions: [ ... ] # condition keys, e.g. geo. operators: [ ... ] # operator keys, e.g. inbound.conn.dscp language: [ ... ] # break, variables, in, else, elif + modifiers: [ ... ] # condition modifiers, e.g. NOT, NOCASE warn: functions: [ ... ] # same categories as deny conditions: [ ... ] @@ -799,9 +800,24 @@ Construct What it controls ``variables`` The entire ``VARS`` and ``SESSION_VARS`` section and all variable usage ``else`` The ``else { ... }`` branch of conditionals ``elif`` The ``elif ... { ... }`` branch of conditionals -``in`` The ``in [...]`` and ``!in [...]`` set membership operators +``in`` Set membership: the ``[...]`` value form and + the ``{...}`` IP range form, negated or not ================ =================================================== +Condition Modifiers +------------------- + +The ``modifiers`` list accepts ``AND``, ``OR``, ``NOT``, ``NOCASE``, ``PRE``, +``SUF``, ``EXT``, ``MID``, ``I``, ``L`` and ``QSA``. Entries match the modifier +however it is written, not only the explicit ``with`` form: ``AND`` also covers +``&&``, ``OR`` also covers ``||``, and ``NOT`` also covers ``!``, ``!=``, +``!~`` and ``!in``. + +Negation that the compiler introduces on its own is not matched. A bare header +test such as ``if inbound.req.X-Foo`` compiles to +``cond %{CLIENT-HEADER:X-Foo} ="" [NOT]``, and denying ``NOT`` does not reject +it — the policy governs what the source writes. + Output ------ diff --git a/tools/hrw4u/src/visitor.py b/tools/hrw4u/src/visitor.py index f3fb38c3a39..edeb3be1253 100644 --- a/tools/hrw4u/src/visitor.py +++ b/tools/hrw4u/src/visitor.py @@ -1019,6 +1019,9 @@ def visitComparison(self, ctx, *, last: bool = False) -> None: else: negate = operator.symbol.type in (hrw4uParser.NEQ, hrw4uParser.NOT_TILDE) + if negate and not self._sandbox_check(ctx, lambda: self._sandbox.check_modifier("NOT")): + return + match ctx: case _ if ctx.value(): rhs = self._get_value_text(ctx.value()) @@ -1041,6 +1044,8 @@ def visitComparison(self, ctx, *, last: bool = False) -> None: cond_txt = f"{lhs} {regex_expr}" case _ if ctx.iprange(): + if not self._sandbox_check(ctx, lambda: self._sandbox.check_language("in")): + return cond_txt = f"{lhs} {ctx.iprange().getText()}" case _ if ctx.set_(): @@ -1138,6 +1143,8 @@ def emit_factor(self, ctx, *, last: bool = False) -> None: match ctx: case _ if ctx.getChildCount() == 2 and ctx.getChild(0).getText() == "!": self._dbg("`NOT' detected") + if not self._sandbox_check(ctx, lambda: self._sandbox.check_modifier("NOT")): + return child = ctx.getChild(1) if child.LPAREN(): self._dbg("GROUP-START (negated)") diff --git a/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.ast.txt b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.ast.txt new file mode 100644 index 00000000000..1fe8d27f00e --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.ast.txt @@ -0,0 +1 @@ +(program (programItem (section REMAP { (sectionBody (conditional (ifStatement if (condition (expression (term (factor inbound.req.X-Foo)))) (block { (blockItem (statement inbound.req.X-Present = (value "1") ;)) })))) })) ) diff --git a/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.input.txt b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.input.txt new file mode 100644 index 00000000000..6afe7ecac39 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.input.txt @@ -0,0 +1,5 @@ +REMAP { + if inbound.req.X-Foo { + inbound.req.X-Present = "1"; + } +} diff --git a/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.output.txt b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.output.txt new file mode 100644 index 00000000000..de2a3891f8c --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.output.txt @@ -0,0 +1,3 @@ +cond %{REMAP_PSEUDO_HOOK} [AND] +cond %{CLIENT-HEADER:X-Foo} ="" [NOT] + set-header X-Present "1" diff --git a/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.sandbox.yaml b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.sandbox.yaml new file mode 100644 index 00000000000..41bcd108005 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/allowed-implicit-not.sandbox.yaml @@ -0,0 +1,6 @@ +sandbox: + message: "Modifier denied by sandbox policy. Contact platform team." + + deny: + modifiers: + - NOT diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.ast.txt b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.ast.txt new file mode 100644 index 00000000000..cc1f544695b --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.ast.txt @@ -0,0 +1 @@ +(program (programItem (section REMAP { (sectionBody (conditional (ifStatement if (condition (expression (term (factor (comparison (comparable inbound.ip) in (iprange { (ip (ipv4 10.0.0.0/8)) })))))) (block { (blockItem (statement inbound.req.X-Internal-Net = (value "1") ;)) })))) (sectionBody (conditional (ifStatement if (condition (expression (term (factor (comparison (comparable inbound.ip) ! in (iprange { (ip (ipv4 192.168.0.0/16)) })))))) (block { (blockItem (statement inbound.req.X-Other-Net = (value "1") ;)) })))) })) ) diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.error.txt b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.error.txt new file mode 100644 index 00000000000..e24836bc1a2 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.error.txt @@ -0,0 +1,3 @@ +Found 2 errors: +'in' is denied by sandbox policy (language) +Feature denied by sandbox policy. Contact platform team. diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.input.txt b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.input.txt new file mode 100644 index 00000000000..3a35da6c886 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.input.txt @@ -0,0 +1,9 @@ +REMAP { + if inbound.ip in {10.0.0.0/8} { + inbound.req.X-Internal-Net = "1"; + } + + if inbound.ip !in {192.168.0.0/16} { + inbound.req.X-Other-Net = "1"; + } +} diff --git a/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.sandbox.yaml b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.sandbox.yaml new file mode 100644 index 00000000000..f10fef32e15 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-language-in-iprange.sandbox.yaml @@ -0,0 +1,6 @@ +sandbox: + message: "Feature denied by sandbox policy. Contact platform team." + + deny: + language: + - in diff --git a/tools/hrw4u/tests/data/sandbox/denied-modifier-not.ast.txt b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.ast.txt new file mode 100644 index 00000000000..a803ec9ec4a --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.ast.txt @@ -0,0 +1 @@ +(program (programItem (section REMAP { (sectionBody (conditional (ifStatement if (condition (expression (term (factor ! (factor (functionCall internal ( ))))))) (block { (blockItem (statement inbound.req.X-Not-Internal = (value "1") ;)) })))) (sectionBody (conditional (ifStatement if (condition (expression (term (factor (comparison (comparable inbound.method) != (value "GET")))))) (block { (blockItem (statement inbound.req.X-Not-Get = (value "1") ;)) })))) (sectionBody (conditional (ifStatement if (condition (expression (term (factor (comparison (comparable inbound.req.Accept-Language) !~ (regex /es-py/)))))) (block { (blockItem (statement inbound.req.X-Not-Matched = (value "1") ;)) })))) (sectionBody (conditional (ifStatement if (condition (expression (term (factor (comparison (comparable inbound.url.path) ! in (set [ (value "php") , (value "html") ])))))) (block { (blockItem (statement inbound.req.X-Not-Listed = (value "1") ;)) })))) })) ) diff --git a/tools/hrw4u/tests/data/sandbox/denied-modifier-not.error.txt b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.error.txt new file mode 100644 index 00000000000..8bc55379ab1 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.error.txt @@ -0,0 +1,3 @@ +Found 4 errors: +'NOT' is denied by sandbox policy (modifier) +Modifier denied by sandbox policy. Contact platform team. diff --git a/tools/hrw4u/tests/data/sandbox/denied-modifier-not.input.txt b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.input.txt new file mode 100644 index 00000000000..87fcc435e05 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.input.txt @@ -0,0 +1,17 @@ +REMAP { + if !internal() { + inbound.req.X-Not-Internal = "1"; + } + + if inbound.method != "GET" { + inbound.req.X-Not-Get = "1"; + } + + if inbound.req.Accept-Language !~ /es-py/ { + inbound.req.X-Not-Matched = "1"; + } + + if inbound.url.path !in ["php", "html"] { + inbound.req.X-Not-Listed = "1"; + } +} diff --git a/tools/hrw4u/tests/data/sandbox/denied-modifier-not.sandbox.yaml b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.sandbox.yaml new file mode 100644 index 00000000000..41bcd108005 --- /dev/null +++ b/tools/hrw4u/tests/data/sandbox/denied-modifier-not.sandbox.yaml @@ -0,0 +1,6 @@ +sandbox: + message: "Modifier denied by sandbox policy. Contact platform team." + + deny: + modifiers: + - NOT From 4d7ecd7c5880d45c6a7ab424a40014bbee1dfe99 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 22 Sep 2026 15:22:26 -0500 Subject: [PATCH 6/8] Handle all cachekey regex capture groups (#13653) Cachekey patterns with ten or more capture groups can crash ATS when building a cache key because a successful match leaves the capture vector empty. Replacement patterns also reject valid group references when the match buffer is too small or trailing optional groups do not participate in a match. This patch sizes match buffers from the validated pattern capture count and checks replacement references at initialization. Unmatched optional groups contribute empty strings. Unit and replay coverage verifies full cache keys across the capture limit and optional-group combinations. Fixes: #13638 Co-authored-by: GPT-6 Astra Light Co-authored-by: GPT-6 Astra Medium Co-authored-by: bneradt (cherry picked from commit f39ee5f5b78722252689c23a46fadd577a2a95ea) --- plugins/cachekey/pattern.cc | 37 ++- plugins/cachekey/pattern.h | 3 +- plugins/cachekey/unit_tests/pattern_test.cc | 42 ++- .../cachekey/cachekey_capture.test.py | 19 ++ .../pluginTest/cachekey/capture.replay.yaml | 294 ++++++++++++++++++ 5 files changed, 376 insertions(+), 19 deletions(-) create mode 100644 tests/gold_tests/pluginTest/cachekey/cachekey_capture.test.py create mode 100644 tests/gold_tests/pluginTest/cachekey/capture.replay.yaml diff --git a/plugins/cachekey/pattern.cc b/plugins/cachekey/pattern.cc index 515cd8f4f0b..45743815e27 100644 --- a/plugins/cachekey/pattern.cc +++ b/plugins/cachekey/pattern.cc @@ -152,7 +152,7 @@ Pattern::process(const String &subject, StringVector &result) } else { /* Replacement was not provided so return all capturing groups except the group zero. */ StringVector captures; - if (capture(subject, captures)) { + if (capture(subject, captures) && !captures.empty()) { if (captures.size() == 1) { result.push_back(captures[0]); } else { @@ -210,7 +210,7 @@ Pattern::capture(const String &subject, StringVector &result) return false; } - RegexMatches matches; + RegexMatches matches(_captureCount + 1); int matchCount = _re.exec(subject, matches, RE_NOTEMPTY); if (matchCount < 0) { if (matchCount != RE_ERROR_NOMATCH) { @@ -219,7 +219,7 @@ Pattern::capture(const String &subject, StringVector &result) return false; } - for (int i = 0; i < matchCount; i++) { + for (int i = 0; i < matches.size(); i++) { std::string_view capture = matches[i]; String dst(capture.data(), capture.length()); @@ -246,7 +246,7 @@ Pattern::replace(const String &subject, String &result) return false; } - RegexMatches matches; + RegexMatches matches(_captureCount + 1); int matchCount = _re.exec(subject, matches, RE_NOTEMPTY); if (matchCount < 0) { if (matchCount != RE_ERROR_NOMATCH) { @@ -255,18 +255,11 @@ Pattern::replace(const String &subject, String &result) return false; } - /* Verify the replacement has the right number of matching groups */ - for (int i = 0; i < _tokenCount; i++) { - if (_tokens[i] >= matchCount) { - CacheKeyError("invalid reference in replacement string: $%d", _tokens[i]); - return false; - } - } - int previous = 0; for (int i = 0; i < _tokenCount; i++) { - int replIndex = _tokens[i]; - std::string_view capture = matches[replIndex]; + int replIndex = _tokens[i]; + // Trailing optional groups may not participate in this match. + std::string_view capture = (replIndex < matches.size()) ? matches[replIndex] : std::string_view{""}; String src(_replacement, _tokenOffset[i], 2); String dst(capture.data(), capture.length()); @@ -304,6 +297,12 @@ Pattern::compile() return false; } + _captureCount = _re.get_capture_count(); + if (_captureCount < 0) { + CacheKeyError("failed to get capture count for pattern '%s'", _pattern.c_str()); + return false; + } + if (!_replace) { /* No replacement necessary - we are done. */ return true; @@ -336,6 +335,16 @@ Pattern::compile() } } + if (success) { + for (int i = 0; i < _tokenCount; i++) { + if (_tokens[i] > _captureCount) { + CacheKeyError("invalid reference $%d in replacement '%s': pattern defines only %d group(s)", _tokens[i], + _replacement.c_str(), _captureCount); + return false; + } + } + } + return success; } diff --git a/plugins/cachekey/pattern.h b/plugins/cachekey/pattern.h index e3f441d27ab..1392849b371 100644 --- a/plugins/cachekey/pattern.h +++ b/plugins/cachekey/pattern.h @@ -50,7 +50,8 @@ class Pattern private: bool compile(); - Regex _re; /**< @brief Regex compiled object */ + Regex _re; /**< @brief Regex compiled object */ + int32_t _captureCount = 0; ///< Number of capture groups defined by the compiled pattern. String _pattern; /**< @brief Regex pattern string, containing regex patterns and capturing groups. */ String diff --git a/plugins/cachekey/unit_tests/pattern_test.cc b/plugins/cachekey/unit_tests/pattern_test.cc index 2b9adb90d42..d0637e0d35d 100644 --- a/plugins/cachekey/unit_tests/pattern_test.cc +++ b/plugins/cachekey/unit_tests/pattern_test.cc @@ -202,13 +202,47 @@ TEST_CASE("Pattern compile and match behavior", "[cachekey][pattern]") CHECK(res == "num=123;"); } - SECTION("Replacement with invalid group reference") + SECTION("Replacement with invalid group reference fails at initialization") { Pattern p; - REQUIRE(p.init("(\\w+)", "$5", true)); // only 2 groups (0 and 1) + + CHECK_FALSE(p.init("(\\w+)", "$5", true)); + CHECK_FALSE(p.init("(a)(b)?", "$3", true)); + CHECK_FALSE(p.init("literal", "$1", true)); + } + + SECTION("Replacement with optional capture groups") + { + Pattern p; + + REQUIRE(p.init("^(a)(b)?(c)?$", "$1-$2-$3", true)); + for (const auto &[subject, expected] : { + std::pair{"a", "a--" }, + {"ab", "a-b-" }, + {"ac", "a--c" }, + {"abc", "a-b-c"} + }) { + String res; + + REQUIRE(p.replace(subject, res)); + CHECK(res == expected); + } + } + + SECTION("Capture and replacement beyond the inline match buffer") + { + Pattern p; + StringVector result; + + REQUIRE(p.init("(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)(k)(l)")); + REQUIRE(p.process("abcdefghijkl", result)); + CHECK(result == StringVector{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l"}); + + REQUIRE(p.init("(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)(k)(l)", "$9$1", true)); String res; - // Should fail because $5 doesn't exist - CHECK(p.replace("test", res) == false); + + REQUIRE(p.replace("abcdefghijkl", res)); + CHECK(res == "ia"); } SECTION("process() method - capture mode (no replacement)") diff --git a/tests/gold_tests/pluginTest/cachekey/cachekey_capture.test.py b/tests/gold_tests/pluginTest/cachekey/cachekey_capture.test.py new file mode 100644 index 00000000000..72451b04403 --- /dev/null +++ b/tests/gold_tests/pluginTest/cachekey/cachekey_capture.test.py @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = 'Verify cachekey patterns preserve all capture groups.' +Test.SkipUnless(Condition.PluginExists('cachekey.so'), Condition.PluginExists('xdebug.so')) +Test.ATSReplayTest(replay_file='capture.replay.yaml') diff --git a/tests/gold_tests/pluginTest/cachekey/capture.replay.yaml b/tests/gold_tests/pluginTest/cachekey/capture.replay.yaml new file mode 100644 index 00000000000..3c5f668f231 --- /dev/null +++ b/tests/gold_tests/pluginTest/cachekey/capture.replay.yaml @@ -0,0 +1,294 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +meta: + version: '1.0' + +autest: + description: 'Cachekey captures beyond the default regex match capacity' + server: + name: server + client: + name: client + ats: + name: ts + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: cachekey + plugin_config: + - 'xdebug.so --enable=x-cache-key' + remap_config: + - from: 'http://nine.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=(a)(b)(c)(d)(e)(f)(g)(h)(i)' + - from: 'http://ten.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)' + - from: 'http://twelve.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)(k)(l)' + - from: 'http://replace.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=/(a)(b)(c)(d)(e)(f)(g)(h)(i)(j)(k)(l)/$9$1/' + - from: 'http://whole.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=abcdef' + - from: 'http://no-match.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=(z)' + - from: 'http://optional.example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: cachekey.so + args: + - '--static-prefix=capture' + - '--capture-path=/(a)(b)?(c)?/$1-$2-$3/' + +sessions: +- transactions: + - client-request: + method: GET + url: /abcdefghi + version: '1.1' + headers: + fields: + - [Host, nine.example.com] + - [uuid, nine] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a/b/c/d/e/f/g/h/i', as: equal}] + + - client-request: + method: GET + url: /abcdefghij + version: '1.1' + headers: + fields: + - [Host, ten.example.com] + - [uuid, ten] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a/b/c/d/e/f/g/h/i/j', as: equal}] + + - client-request: + method: GET + url: /abcdefghijkl + version: '1.1' + headers: + fields: + - [Host, twelve.example.com] + - [uuid, twelve] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a/b/c/d/e/f/g/h/i/j/k/l', as: equal}] + + - client-request: + method: GET + url: /abcdefghijkl + version: '1.1' + headers: + fields: + - [Host, replace.example.com] + - [uuid, replace] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/ia', as: equal}] + + - client-request: + method: GET + url: /abcdef + version: '1.1' + headers: + fields: + - [Host, whole.example.com] + - [uuid, whole] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/abcdef', as: equal}] + + - client-request: + method: GET + url: /abcdef + version: '1.1' + headers: + fields: + - [Host, no-match.example.com] + - [uuid, no-match] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture', as: equal}] + + - client-request: + method: GET + url: /a + version: '1.1' + headers: + fields: + - [Host, optional.example.com] + - [uuid, optional-a] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a--', as: equal}] + + - client-request: + method: GET + url: /ab + version: '1.1' + headers: + fields: + - [Host, optional.example.com] + - [uuid, optional-ab] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a-b-', as: equal}] + + - client-request: + method: GET + url: /ac + version: '1.1' + headers: + fields: + - [Host, optional.example.com] + - [uuid, optional-ac] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a--c', as: equal}] + + - client-request: + method: GET + url: /abc + version: '1.1' + headers: + fields: + - [Host, optional.example.com] + - [uuid, optional-abc] + - [X-Debug, x-cache-key] + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 0] + proxy-response: + status: 200 + headers: + fields: + - [X-Cache-Key, {value: '/capture/a-b-c', as: equal}] From f4fce0f939825d6c48e346d025dda7b23f1b55c0 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 22 Sep 2026 16:56:32 -0500 Subject: [PATCH 7/8] Test 103 Early Hints before a cached range response (#13715) Issue #12244 reported ATS aborting on the HttpTunnel skip_bytes assertion for Range requests of a small, cacheable 308. The fix in #12906 was tested only with a 100 Continue ahead of a compressed POST response, so the reported scenario had no coverage. 103 Early Hints take the same interim-response path as 100 Continue, and a Range miss with cache.range.write enabled caches the untransformed response behind the range transform, leaving the cache-write consumer with a stale header size to skip in a body-only buffer. This patch extends the regression test with a run in which the origin sends a 103 Early Hints and then a 65 byte, cacheable 308 that the client requests with "Range: bytes=0-64". Without the #12906 fix, this run hits the same assertion reported in the issue; with it, ATS keeps running. Fixes: #12244 Co-authored-by: Claude Opus 5.5 (Medium effort) (cherry picked from commit 0079038a12a9f1b6a71c8c4b5e97b953ea7c4663) --- .../compress-cache-untransformed.test.py | 80 ++++++++++++++----- .../compress/compress_100_continue_origin.py | 45 +++++++++-- 2 files changed, 100 insertions(+), 25 deletions(-) diff --git a/tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py b/tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py index bf5e0a85ca3..586dc54eca1 100644 --- a/tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py +++ b/tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py @@ -18,6 +18,13 @@ This test uses a custom origin that sends "100 Continue" followed by a compressible, non-chunked 200 OK to trigger the exact crash path. + +The issue itself was reported for range requests of a small, cacheable 308 +response. 103 Early Hints responses are forwarded to HTTP/1.1 clients through +the same setup_100_continue_transfer() path, and a cache miss for a Range +request installs the range transform while caching the untransformed +response. The second test run reproduces that scenario: 103 Early Hints, then +a 65 byte 308, requested with "Range: bytes=0-64". ''' # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file @@ -40,8 +47,9 @@ from ports import get_port Test.Summary = ''' -Regression test for compress plugin with cache=true causing assertion failure -when origin sends 100 Continue before a compressible response (#12244) +Regression test for a cache-write assertion failure when the origin sends an +interim response (100 Continue or 103 Early Hints) before a response that goes +through a transform (#12244) ''' Test.SkipUnless(Condition.PluginExists('compress.so')) @@ -51,31 +59,35 @@ class CompressCacheUntransformedTest: def __init__(self): self.setupTS() - self.run() + continue_tr = Test.AddTestRun("100 Continue before a compressed, cached response") + early_hints_tr = Test.AddTestRun("103 Early Hints before a range requested, cached 308") + self.continue_origin = self._makeOrigin(continue_tr, "continue_origin", "continue") + self.early_hints_origin = self._makeOrigin(early_hints_tr, "early_hints_origin", "early-hints") + self.configureTS() + self.run100Continue(continue_tr) + self.runEarlyHintsRange(early_hints_tr) def setupTS(self): self.ts = Test.MakeATSProcess("ts", enable_cache=True) - def run(self): - tr = Test.AddTestRun() - - # Copy scripts into the test run directory. + def _makeOrigin(self, tr, name, mode): tr.Setup.CopyAs("compress_100_continue_origin.py") tr.Setup.Copy("etc/compress-cache-false.config") - # Create and configure the custom origin server process. - origin = tr.Processes.Process("origin") - origin_port = get_port(origin, 'http_port') + origin = tr.Processes.Process(name) + port = get_port(origin, 'http_port') origin.Command = (f'{sys.executable} compress_100_continue_origin.py' - f' --port {origin_port}') - origin.Ready = When.PortOpenv4(origin_port) + f' --port {port} --mode {mode}') + origin.Ready = When.PortOpenv4(port) origin.ReturnCode = 0 + tr.Processes.Default.StartBefore(origin) + return origin - # Configure ATS. + def configureTS(self): self.ts.Disk.records_config.update( { "proxy.config.diags.debug.enabled": 1, - "proxy.config.diags.debug.tags": "http|compress|http_tunnel", + "proxy.config.diags.debug.tags": "http|compress|http_tunnel|http_range", # Do NOT send 100 Continue from ATS - let the origin send it. # This ensures ATS processes the origin's 100 via # handle_100_continue_response -> setup_100_continue_transfer, @@ -85,13 +97,25 @@ def run(self): # the cache write path where the stale client_response_hdr_bytes # causes the crash. "proxy.config.http.cache.post_method": 1, + # Cache the response to a Range request on a miss so that the + # range transform is installed alongside an untransformed + # cache write. + "proxy.config.http.cache.range.write": 1, }) - self.ts.Disk.remap_config.AddLine( - f'map / http://127.0.0.1:{origin_port}/' - f' @plugin=compress.so' - f' @pparam={Test.RunDirectory}/compress-cache-false.config') - + early_hints_port = self.early_hints_origin.Variables.http_port + continue_port = self.continue_origin.Variables.http_port + self.ts.Disk.remap_config.AddLines( + [ + f'map /early-hints/ http://127.0.0.1:{early_hints_port}/early-hints/' + f' @plugin=compress.so' + f' @pparam={Test.RunDirectory}/compress-cache-false.config', + f'map / http://127.0.0.1:{continue_port}/' + f' @plugin=compress.so' + f' @pparam={Test.RunDirectory}/compress-cache-false.config', + ]) + + def run100Continue(self, tr): # Client sends a POST with Expect: 100-continue but does not wait for # the 100 response before sending the body (--expect100-timeout 0). # The crash is triggered by ATS processing the origin's 100 Continue, @@ -106,7 +130,6 @@ def run(self): f' --data "test body data"' f' http://127.0.0.1:{self.ts.Variables.port}/test/resource.js') client.ReturnCode = 0 - client.StartBefore(origin) client.StartBefore(self.ts) # The key assertion: ATS must still be running after the test. @@ -114,5 +137,22 @@ def run(self): # in HttpTunnel::producer_run. tr.StillRunningAfter = self.ts + def runEarlyHintsRange(self, tr): + # The range covers the entire 65 byte 308 body, as in the issue. On a + # cache miss ATS installs the range transform and caches the + # untransformed response, so the cache-write consumer reads from the + # body-only server-to-transform buffer. + client = tr.Processes.Default + client.Command = ( + f'curl --http1.1 -s -o /dev/null' + f' -H "Range: bytes=0-64"' + f' http://127.0.0.1:{self.ts.Variables.port}/early-hints/redirect') + client.ReturnCode = 0 + + # Without the fix, ATS crashes with a failed assertion in + # HttpTunnel::producer_run because the cache-write consumer skips the + # size of the forwarded 103 headers in a buffer holding only the body. + tr.StillRunningAfter = self.ts + CompressCacheUntransformedTest() diff --git a/tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py b/tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py index a3b0f29b01e..da2c7ada174 100644 --- a/tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py +++ b/tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py @@ -1,8 +1,12 @@ #!/usr/bin/env python3 -"""Origin server that sends 100 Continue then a compressible 200 OK. +"""Origin server that sends an interim 1xx response before the final response. -Used to reproduce the crash in HttpTunnel::producer_run when compress.so -with cache=true is combined with a 100 Continue response from the origin. +Used to reproduce the crash in HttpTunnel::producer_run when a response +transform is combined with an interim response from the origin: + +* continue: 100 Continue followed by a compressible 200 OK. +* early-hints: 103 Early Hints followed by a small, cacheable 308, as in + https://github.com/apache/trafficserver/issues/12244. """ # Licensed to the Apache Software Foundation (ASF) under one @@ -25,11 +29,13 @@ import signal import socket import sys +import time def parse_args(): parser = argparse.ArgumentParser() parser.add_argument('--port', type=int, required=True, help='Port to listen on') + parser.add_argument('--mode', choices=['continue', 'early-hints'], default='continue', help='Which interim response to send') return parser.parse_args() @@ -44,7 +50,32 @@ def read_request(conn): return data -def handle_connection(conn, addr): +def send_early_hints_and_redirect(conn): + """Send 103 Early Hints followed by a small, cacheable 308. + + The body must be SMALLER than the forwarded 103 response headers so that + a stale skip_bytes exceeds read_avail() in producer_run. The pause lets + ATS finish forwarding the 103 before it reads the 308. + """ + conn.sendall( + b'HTTP/1.1 103 Early Hints\r\n' + b'Link: ; rel=preload; as=style\r\n' + b'Link: ; rel=preload; as=script\r\n' + b'Link: ; rel=preload; as=font; crossorigin\r\n' + b'\r\n') + time.sleep(0.5) + + body = b'Redirecting to https://example.com/new/location/for/the/resource\n' + conn.sendall( + b'HTTP/1.1 308 Permanent Redirect\r\n' + b'Location: https://example.com/new/location/for/the/resource\r\n' + b'Content-Type: text/plain\r\n' + b'Cache-Control: public, max-age=3600\r\n' + b'Content-Length: ' + str(len(body)).encode() + b'\r\n' + b'\r\n' + body) + + +def handle_connection(conn, addr, mode): """Handle a single client connection.""" try: conn.settimeout(10) @@ -52,6 +83,10 @@ def handle_connection(conn, addr): if request is None: return + if mode == 'early-hints': + send_early_hints_and_redirect(conn) + return + # Send 100 Continue immediately. conn.sendall(b'HTTP/1.1 100 Continue\r\n\r\n') @@ -114,7 +149,7 @@ def main(): while True: try: conn, addr = sock.accept() - handle_connection(conn, addr) + handle_connection(conn, addr, args.mode) except socket.timeout: break From 56eb820a7c77b713613a84d3a19e23b2aab09edc Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Tue, 22 Sep 2026 18:17:23 -0500 Subject: [PATCH 8/8] cache: retry without O_DIRECT for an explicit file span (#13718) The non-direct-IO retry added in TS-1312 only fires when O_CREAT is set, and O_CREAT is set only when the span names a directory, so that cache.db can be created inside it. An explicit file span in storage.config therefore gets no retry: on a file system without O_DIRECT support the span open fails outright and the cache is disabled, even though the very same file system works when the span is written as a directory. Whether direct I/O is usable is a property of the file system, not of the form the span was written in. The cache_shm_* autests are the only ones configuring an explicit file span, so on a sandbox whose file system lacks O_DIRECT all of them fail with "must be placed on a file system that supports direct I/O" followed by "Cache Disabled". (cherry picked from commit 3e10681a9e0b863dbc86ea0655a2c57920b35cbd) --- src/iocore/cache/CacheProcessor.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/iocore/cache/CacheProcessor.cc b/src/iocore/cache/CacheProcessor.cc index 59a4ed7f2ac..79d262f3880 100644 --- a/src/iocore/cache/CacheProcessor.cc +++ b/src/iocore/cache/CacheProcessor.cc @@ -249,11 +249,14 @@ CacheProcessor::start_internal(int flags) #endif int64_t blocks = span->blocks; - if (fd < 0 && (opts & O_CREAT)) { // Try without O_DIRECT if this is a file on filesystem, e.g. tmpfs. + // Try again without O_DIRECT (and O_DSYNC) for a span on a file system that does not support it, e.g. tmpfs. This retry + // applies to an explicit file span as much as to a directory span: the file system decides whether direct I/O is usable, + // not the form the span was written in. O_CREAT is carried over only when it was requested above. + if (fd < 0) { #ifdef AIO_FAULT_INJECTION - fd = aioFaultInjection.open(paths[gndisks], DEFAULT_CACHE_OPTIONS | O_CREAT, 0644); + fd = aioFaultInjection.open(paths[gndisks], DEFAULT_CACHE_OPTIONS | (opts & O_CREAT), 0644); #else - fd = open(paths[gndisks], DEFAULT_CACHE_OPTIONS | O_CREAT, 0644); + fd = open(paths[gndisks], DEFAULT_CACHE_OPTIONS | (opts & O_CREAT), 0644); #endif }