Copilot commented on code in PR #13404:
URL: https://github.com/apache/trafficserver/pull/13404#discussion_r3609416052
##########
src/tscore/Encoding.cc:
##########
@@ -77,7 +77,7 @@ escapify_url_common(Arena *arena, char *url, size_t len_in,
int *len_out, char *
static char hex_digit[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8',
'9', 'A', 'B', 'C', 'D', 'E', 'F'};
- if (!url || (dst && dst_size < len_in)) {
+ if (!url || (dst && dst_size < len_in + 1)) {
Review Comment:
The destination-size guard does `len_in + 1`, which can overflow for very
large `len_in` (e.g., from a signed-to-unsigned conversion). That would bypass
the size check and could lead to an out-of-bounds read/write in the fast path.
Use an overflow-safe comparison instead (equivalent for all normal values).
##########
src/tscore/unit_tests/test_Encoding.cc:
##########
@@ -57,6 +57,29 @@ TEST_CASE("Encoding pure escapify url", "[pure_esc_url]")
}
}
+TEST_CASE("Encoding escapify url without a terminator",
"[esc_url_unterminated]")
+{
+ // The source is a counted string, not a C string, so nothing may be read
past len_in.
+ // Sized exactly so that a read past the end is caught by a sanitizer.
+ constexpr std::string_view src{"abcdef"};
+
+ char *unterminated = static_cast<char *>(std::malloc(src.size()));
+ std::memcpy(unterminated, src.data(), src.size());
+
+ char output[128];
+ int output_len;
+
+ REQUIRE(Encoding::pure_escapify_url(nullptr, unterminated, src.size(),
&output_len, output, sizeof(output)) != nullptr);
+ CHECK(output_len == static_cast<int>(src.size()));
+ CHECK(std::string_view(output, output_len) == src);
+
+ REQUIRE(Encoding::escapify_url(nullptr, unterminated, src.size(),
&output_len, output, sizeof(output)) != nullptr);
+ CHECK(output_len == static_cast<int>(src.size()));
+ CHECK(std::string_view(output, output_len) == src);
+
+ std::free(unterminated);
Review Comment:
The test manually malloc/free's `unterminated`. If a `REQUIRE` fails, the
function exits early and the `std::free` is skipped, which can show up as a
leak under LeakSanitizer. Also, if `malloc` ever returns null, `memcpy` would
be UB. Wrap the allocation in a small RAII helper and assert allocation
succeeded; while here, also validate that the API terminates the output at
`output_len`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
