bryancall commented on code in PR #12985:
URL: https://github.com/apache/trafficserver/pull/12985#discussion_r2989641485
##########
src/proxy/ParentSelection.cc:
##########
@@ -556,6 +557,11 @@ ParentRecord::ProcessParents(char *val, bool isPrimary)
errPtr = "Parent string is empty";
goto MERROR;
}
+ if (tmp3 && strlen(tmp3) > MAXDNAME) {
+ errPtr = "Parent hash string is too long";
+ goto MERROR;
Review Comment:
`tmp3` points at the `&` separator, so `strlen(tmp3)` includes it. The
destination buffer is `hash_string[MAXDNAME + 1]`, which can hold a payload of
up to `MAXDNAME` characters plus the NUL terminator. The current check rejects
a hash of exactly `MAXDNAME` characters even though it fits.
This should check the payload length, not the separator-inclusive length:
```cpp
// tmp3 points at the '&' separator; skip it to measure just the hash
payload.
// The destination buffer is hash_string[MAXDNAME + 1], so the payload must
fit in MAXDNAME.
if (tmp3 && strlen(tmp3 + 1) > MAXDNAME) {
```
And the test boundaries need to shift accordingly:
```cpp
// Test 214 — hash string exceeding MAXDNAME should be rejected
std::string long_hash(MAXDNAME + 1, 'a');
// Test 215 — hash string of exactly MAXDNAME should be accepted
std::string max_hash(MAXDNAME, 'b');
```
--
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]
