Copilot commented on code in PR #13608:
URL: https://github.com/apache/trafficserver/pull/13608#discussion_r4118634996
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -310,18 +329,14 @@ inline void
dir_clean_bucket(Dir *b, int s, StripeSM *stripe)
{
Dir *e = b, *p = nullptr;
- Dir *seg = stripe->directory.get_segment(s);
-#ifdef LOOP_CHECK_MODE
- int loop_count = 0;
-#endif
+ Dir *seg = stripe->directory.get_segment(s);
+ int loop_count = 0;
do {
-#ifdef LOOP_CHECK_MODE
- loop_count++;
- if (loop_count > DIR_LOOP_THRESHOLD) {
- if (stripe->directory.bucket_loop_fix(b, s))
- return;
+ // Past the longest legitimate chain, so the chain is corrupt.
+ if (++loop_count > stripe->directory.max_bucket_depth()) {
+ stripe->directory.bucket_loop_fix(b, s, stripe);
+ return;
}
Review Comment:
If the overlong chain is acyclic, `bucket_loop_fix()` returns 0, but this
unconditional return leaves `dir_clean_bucket()` having cleaned neither the
remainder of the chain nor the segment. The new `max_bucket_depth()` contract
explicitly allows an acyclic link into another bucket's row 0, so this path can
leave a still-corrupt chain after cleanup and cause later bounded lookups to
keep failing. Preserve the prior behavior by continuing the walk after the
exact loop check proves it is acyclic (reset the counter to avoid repeatedly
invoking the check).
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -567,7 +575,15 @@ Directory::insert(const CacheKey *key, StripeSM *stripe,
Dir *to_part)
do {
prev = last;
last = next_dir(last, seg);
- } while (last && (++l <= this->buckets * DIR_DEPTH));
+ // Past the longest legitimate chain, so the chain is corrupt. This is
insert()'s only walk, so it is where a
+ // writer meets one: repair and start over rather than linking into a
cycle.
+ if (++l > this->max_bucket_depth()) {
Review Comment:
This cap is bypassed by the `DEBUG`/`DO_CHECK_DIR_FAST` validation earlier
in `insert()`: its `while (col)` walk follows `next_dir()` without a bound
before execution reaches this loop. A debug build enabling that check can still
spin on the same cycle, so the insert path is not bounded in all builds; apply
the same protection to that validation or remove the unbounded walk.
--
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]
