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/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst index 049454f33c0..b6953df6415 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) ^^^^^^^^^^^^^^^^^^^^ @@ -697,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: [ ... ] @@ -778,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/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/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/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 } 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] = ""; 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); +} 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}] 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 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/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/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/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/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-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/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 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) 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."""