Copilot commented on code in PR #12710:
URL: https://github.com/apache/trafficserver/pull/12710#discussion_r2582740121
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -816,6 +818,15 @@ CacheVC::openReadStartEarliest(int /* event ATS_UNUSED */,
Event * /* e ATS_UNUS
dir_lookaside_probe(&key, stripe, &earliest_dir, nullptr)) {
dir = earliest_dir;
if ((ret = do_read_call(&key)) == EVENT_RETURN) {
+ if (this->handler ==
reinterpret_cast<ContinuationHandler>(&CacheVC::openReadStartEarliest)) {
+ is_recursive_call = true;
+ if (++recursive > 10) {
+ char tmpstring[CRYPTO_HEX_SIZE];
+ Error("Too many recursive call with %s", key.toHexStr(tmpstring));
Review Comment:
Critical bug: When the recursion limit is exceeded, the code increments
`recursive` to 11 (line 823) and then jumps to `Ldone` which calls
`free_CacheVC(this)` (line 898). However, we're currently in a recursive call
stack. After `free_CacheVC` returns and the function returns, the previous
stack frame(s) will resume at line 902 and try to decrement `recursive` on the
already-freed object, causing undefined behavior (use-after-free).
The correct approach is to decrement the counter before going to the error
path:
```cpp
if (++recursive > 10) {
char tmpstring[CRYPTO_HEX_SIZE];
Error("Too many recursive call with %s", key.toHexStr(tmpstring));
--recursive; // Decrement before error path
goto Ldone;
}
```
This ensures that when the calling frame resumes after this function
returns, the recursion counter is balanced.
```suggestion
Error("Too many recursive call with %s",
key.toHexStr(tmpstring));
--recursive; // Decrement before error path
```
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -749,8 +749,10 @@ Lread: {
int
CacheVC::openReadStartEarliest(int /* event ATS_UNUSED */, Event * /* e
ATS_UNUSED */)
{
- int ret = 0;
- Doc *doc = nullptr;
+ int ret = 0;
+ int res = 0;
Review Comment:
[nitpick] The variable names `ret` and `res` are very similar and serve
similar purposes (both store return values), which could lead to confusion.
Consider using more descriptive names to clarify their different roles:
- `ret` is used for the immediate return value from `do_read_call()` or
`do_write_call()`
- `res` is used to store the return value from the recursive `handleEvent()`
call
For example: `call_result` and `event_result`, or `immediate_ret` and
`recursive_ret`.
```suggestion
int call_result = 0;
int event_result = 0;
```
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -816,6 +818,15 @@ CacheVC::openReadStartEarliest(int /* event ATS_UNUSED */,
Event * /* e ATS_UNUS
dir_lookaside_probe(&key, stripe, &earliest_dir, nullptr)) {
dir = earliest_dir;
if ((ret = do_read_call(&key)) == EVENT_RETURN) {
+ if (this->handler ==
reinterpret_cast<ContinuationHandler>(&CacheVC::openReadStartEarliest)) {
+ is_recursive_call = true;
+ if (++recursive > 10) {
+ char tmpstring[CRYPTO_HEX_SIZE];
+ Error("Too many recursive call with %s", key.toHexStr(tmpstring));
Review Comment:
The error message "Too many recursive call with %s" has a grammatical error.
It should be either "Too many recursive calls" (plural) or "Too many recursive
call attempts". Consider changing to: "Too many recursive calls with %s" or
"Recursion limit exceeded for %s"
```suggestion
Error("Too many recursive calls with %s",
key.toHexStr(tmpstring));
```
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -816,6 +818,15 @@ CacheVC::openReadStartEarliest(int /* event ATS_UNUSED */,
Event * /* e ATS_UNUS
dir_lookaside_probe(&key, stripe, &earliest_dir, nullptr)) {
dir = earliest_dir;
if ((ret = do_read_call(&key)) == EVENT_RETURN) {
+ if (this->handler ==
reinterpret_cast<ContinuationHandler>(&CacheVC::openReadStartEarliest)) {
+ is_recursive_call = true;
+ if (++recursive > 10) {
Review Comment:
[nitpick] Consider defining a named constant for the recursion limit (10) to
improve code maintainability and make it easier to adjust if needed. For
example:
```cpp
constexpr int MAX_READ_RECURSION_DEPTH = 10;
```
This would make the code more self-documenting and easier to configure if
the limit needs to be adjusted in the future.
--
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]
