masaori335 commented on code in PR #13835:
URL: https://github.com/apache/trafficserver/pull/13835#discussion_r4396175661
##########
src/proxy/logging/unit-tests/test_LogAccess.cc:
##########
@@ -245,3 +245,37 @@ TEST_CASE("LogAccess marshals response HTTP versions as
compact strings", "[LogA
check([&](char *buf) { return access.marshal_server_resp_http_version(buf);
});
check([&](char *buf) { return access.marshal_cache_resp_http_version(buf);
});
}
+
+TEST_CASE("LogAccess JSON string slice stays within the raw value",
"[LogAccess]")
+{
+ // A value made of control bytes escapes to six times its raw length. A slice
+ // offset expressed in that escaped space must not be used to index the raw
+ // marshalled value, or the read runs well past its end.
+ constexpr int val_len = 10;
+ std::vector<char> src(INK_MIN_ALIGN * 2, '\x7e'); // sentinel fills the
padding
+ for (int i = 0; i < val_len; ++i) {
+ src[i] = '\x01';
+ }
+ src[val_len] = '\0';
+
+ char dest[256];
+
+ SECTION("a slice that only overlaps the escaped output selects nothing")
+ {
+ char spec[] = "x[50:60]"; // LogSlice rewrites the notation in place
+ LogSlice slice(spec);
+ char *ptr = src.data();
+ int n = LogAccess::unmarshal_str(&ptr, dest, sizeof(dest), &slice,
LOG_ESCAPE_JSON);
+ CHECK(n == 0);
+ }
+
+ SECTION("an in-range slice escapes only the selected raw bytes")
Review Comment:
With the documented semantics, `[2:5]` yields nothing (a 6-byte escape
doesn't fit in 3). These two cases cover the truncation rule and the read bound
instead. Comparing against `std::string_view(dest, n)` also avoids reading
uninitialized `dest` when `n < 6`, which the current `std::string_view(dest,
6)` does:
```suggestion
SECTION("the escaped output is cut at end - start, dropping a partial
escape")
{
char spec[] = "x[0:14]";
LogSlice slice(spec);
char *ptr = src.data();
int n = LogAccess::unmarshal_str(&ptr, dest, sizeof(dest),
&slice, LOG_ESCAPE_JSON);
CHECK(std::string_view(dest, n) == "\\u0001\\u0001");
}
SECTION("a slice reaching past the raw value reads only up to its end")
{
char spec[] = "x[8:60]";
LogSlice slice(spec);
char *ptr = src.data();
int n = LogAccess::unmarshal_str(&ptr, dest, sizeof(dest),
&slice, LOG_ESCAPE_JSON);
CHECK(std::string_view(dest, n) == "\\u0001\\u0001");
}
```
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -889,14 +895,17 @@ unmarshal_str_json(char **buf, char *dest, int len,
LogSlice *slice)
char *val_buf = *buf;
int val_len = static_cast<int>(::strlen(val_buf));
- int escaped_len = escape_json(nullptr, val_buf, val_len);
+ int escaped_len = escape_json(nullptr, val_buf, val_len, 0);
*buf += LogAccess::padded_strlen(val_buf); // this is how it was stored
if (slice && slice->m_enable) {
int offset, n;
- n = slice->toStrOffset(escaped_len, &offset);
+ // Slice against the raw value length, like unmarshal_str does. Using the
Review Comment:
This keeps the documented semantics (raw `start`, escaped output capped at
`end - start`) and bounds the read by the raw bytes left after `offset`:
```suggestion
// start is a raw position, while n caps the escaped output (see "Log
Field Slicing" in the docs).
n = slice->toStrOffset(escaped_len, &offset);
Dbg(dbg_ctl_log_escape, "unmarshal_str_json start, n=%d, offset=%d", n,
offset);
if (n <= 0 || offset >= val_len) {
return 0;
}
if (n >= len) {
DBG_UNMARSHAL_DEST_OVERRUN
return -1;
}
return escape_json(dest, (val_buf + offset), val_len - offset, n);
```
--
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]
