cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5512256169
Follow-up on a review question: `allocated()` indexes `_blobs` with a blob index taken from a caller-supplied id and does not range check it. That is in fact safe, but only by coincidence. `_splitID` masks the blob index with `METRIC_TYPE_MASK` (`0x1FFF`), and `MAX_BLOBS` is `8192`, so the index is always a valid `_blobs` subscript by construction rather than by a check. Nothing in the code tied those two constants together, and every other accessor that splits an id — `lookup`, `name`, `rename`, `valid` — depends on the same relationship. b4fee22167 makes it explicit: ```cpp static_assert(MAX_BLOBS == METRIC_TYPE_MASK + 1, "a masked blob index must always be a valid _blobs index"); ``` Verified it fires: dropping `MAX_BLOBS` to 4096 fails the build rather than silently producing out-of-range subscripts throughout the class. Also added a test for the largest possible id, which exercises the other half — the offset is *not* masked to the blob size, so it needs the explicit `entry >= MAX_SIZE` check to avoid running off the end of a blob. Worth noting for a possible follow-up, out of scope here: `METRIC_TYPE_MASK` is misnamed. It has exactly one use, masking the blob index in `_splitID`, and has nothing to do with the metric type, which lives at `METRIC_TYPE_BITS`. Renaming it would be a one-line change but it is a public constant, so I left it alone. -- 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]
