From c400c6e291c000246fcfa77ff63968a096ac24ba Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 1 Oct 2025 12:54:10 +0200 Subject: [PATCH 1/4] obj: Return BOC state reached from ObjWaitState() This will allow safe checks related to BOC states that are not guarded by the BOC lock. --- bin/varnishd/cache/cache_obj.c | 6 +++++- bin/varnishd/cache/cache_varnishd.h | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bin/varnishd/cache/cache_obj.c b/bin/varnishd/cache/cache_obj.c index 804c226aa6..6cf110300c 100644 --- a/bin/varnishd/cache/cache_obj.c +++ b/bin/varnishd/cache/cache_obj.c @@ -524,9 +524,10 @@ ObjSetState(struct worker *wrk, struct objcore *oc, enum boc_state_e next, /*==================================================================== */ -void +enum boc_state_e ObjWaitState(const struct objcore *oc, enum boc_state_e want) { + enum boc_state_e got; CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); CHECK_OBJ_NOTNULL(oc->boc, BOC_MAGIC); @@ -540,7 +541,10 @@ ObjWaitState(const struct objcore *oc, enum boc_state_e want) break; (void)Lck_CondWait(&oc->boc->cond, &oc->boc->mtx); } + got = oc->boc->state; Lck_Unlock(&oc->boc->mtx); + + return (got); } /*==================================================================== diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h index 56ce1c14bc..a2edf20833 100644 --- a/bin/varnishd/cache/cache_varnishd.h +++ b/bin/varnishd/cache/cache_varnishd.h @@ -346,7 +346,7 @@ uint64_t ObjWaitExtend(const struct worker *, const struct objcore *, uint64_t l, enum boc_state_e *statep); void ObjSetState(struct worker *, struct objcore *, enum boc_state_e next, unsigned broadcast); -void ObjWaitState(const struct objcore *, enum boc_state_e want); +enum boc_state_e ObjWaitState(const struct objcore *, enum boc_state_e want); void ObjTouch(struct worker *, struct objcore *, vtim_real now); void ObjFreeObj(struct worker *, struct objcore *); void ObjSlim(struct worker *, struct objcore *); From 723da9add515683efd405e7e58c5308a71481cde Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 1 Oct 2025 13:51:00 +0200 Subject: [PATCH 2/4] cocci: Inspect BOC state from ObjWaitState() After returning from ObjWaitState(), replace any access of oc->boc->state with the one that was captured by the wait loop. --- tools/coccinelle/obj_wait_state_check.cocci | 39 +++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 tools/coccinelle/obj_wait_state_check.cocci diff --git a/tools/coccinelle/obj_wait_state_check.cocci b/tools/coccinelle/obj_wait_state_check.cocci new file mode 100644 index 0000000000..085a6182c8 --- /dev/null +++ b/tools/coccinelle/obj_wait_state_check.cocci @@ -0,0 +1,39 @@ +/* + * Inspect the state returned by ObjWaitState(). + */ + +@capture_state@ +identifier func; +expression oc; +@@ + + func(...) + { + ... +-ObjWaitState(oc, ++state = ObjWaitState(oc, + ...); + ... +-oc->boc->state ++state + ... + } + +@declare_state@ +identifier capture_state.func; +@@ + + func(...) + { ++enum boc_state_e state; + ... + } + +@ignore_state@ +statement stmt; +@@ + +<...stmt...> +-ObjWaitState( ++(void)ObjWaitState( + ...); From c7e5ad59eb249249da0f7c6a83f6fcde23685bac Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Fri, 3 Oct 2025 10:23:55 +0200 Subject: [PATCH 3/4] varnishd: Apply obj_wait_state_check.cocci --- bin/varnishd/cache/cache_esi_deliver.c | 2 +- bin/varnishd/cache/cache_fetch.c | 9 +++++---- bin/varnishd/cache/cache_hash.c | 2 +- bin/varnishd/cache/cache_req_fsm.c | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/bin/varnishd/cache/cache_esi_deliver.c b/bin/varnishd/cache/cache_esi_deliver.c index ccb44d32d3..06daeb9586 100644 --- a/bin/varnishd/cache/cache_esi_deliver.c +++ b/bin/varnishd/cache/cache_esi_deliver.c @@ -911,7 +911,7 @@ ved_deliver(struct req *req, int wantbody) /* OA_GZIPBITS are not valid until BOS_FINISHED */ if (req->boc != NULL) - ObjWaitState(req->objcore, BOS_FINISHED); + (void)ObjWaitState(req->objcore, BOS_FINISHED); if (req->objcore->flags & OC_F_FAILED) { /* No way of signalling errors in the middle of diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index 55374cf091..22afe6b341 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -886,7 +886,7 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo) VSLb(bo->vsl, SLT_Notice, "vsl: Conditional fetch wait for streaming object"); /* XXX: We should have a VCL controlled timeout here */ - ObjWaitState(stale_oc, BOS_FINISHED); + (void)ObjWaitState(stale_oc, BOS_FINISHED); stale_state = stale_boc->state; HSH_DerefBoc(bo->wrk, stale_oc); stale_boc = NULL; @@ -1172,6 +1172,7 @@ void VBF_Fetch(struct worker *wrk, struct req *req, struct objcore *oc, struct objcore *oldoc, enum vbf_fetch_mode_e mode) { + enum boc_state_e state; struct boc *boc; struct busyobj *bo; enum task_prio prio; @@ -1257,12 +1258,12 @@ VBF_Fetch(struct worker *wrk, struct req *req, struct objcore *oc, THR_SetBusyobj(NULL); bo = NULL; /* ref transferred to fetch thread */ if (mode == VBF_BACKGROUND) { - ObjWaitState(oc, BOS_REQ_DONE); + (void)ObjWaitState(oc, BOS_REQ_DONE); (void)VRB_Ignore(req); } else { - ObjWaitState(oc, BOS_STREAM); + state = ObjWaitState(oc, BOS_STREAM); AZ(oc->flags & OC_F_BUSY); - if (oc->boc->state == BOS_FAILED) + if (state == BOS_FAILED) AN(oc->flags & OC_F_FAILED); } } diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index a72abd8cc3..7191764543 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -990,7 +990,7 @@ HSH_Cancel(struct worker *wrk, struct objcore *oc, struct boc *boc) if (boc != NULL) { hsh_cancel(oc); - ObjWaitState(oc, BOS_FINISHED); + (void)ObjWaitState(oc, BOS_FINISHED); } if (bocref != NULL) diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index 9dc0db092b..f4d3810a27 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -448,7 +448,7 @@ cnt_transmit(struct worker *wrk, struct req *req) /* Grab a ref to the bo if there is one (=streaming) */ req->boc = HSH_RefBoc(req->objcore); if (req->boc && req->boc->state < BOS_STREAM) - ObjWaitState(req->objcore, BOS_STREAM); + (void)ObjWaitState(req->objcore, BOS_STREAM); clval = http_GetContentLength(req->resp); /* RFC 7230, 3.3.3 */ status = http_GetStatus(req->resp); From ad2a6684975402452cdb4c96248723bcf8d0a9d0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Fri, 3 Oct 2025 10:28:35 +0200 Subject: [PATCH 4/4] fetch: Atomically capture stale_oc state It should be safe as it was because there shouldn't be a transition from BOS_FINISHED to BOS_FAILED, but now that it can be atomic, it should not only be safe but also future-proof. It originally matched ignore_state from the coccinelle patch because of the separate boc variable, and had to be manually edited. --- bin/varnishd/cache/cache_fetch.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index 22afe6b341..aff4a44a13 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -886,8 +886,7 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo) VSLb(bo->vsl, SLT_Notice, "vsl: Conditional fetch wait for streaming object"); /* XXX: We should have a VCL controlled timeout here */ - (void)ObjWaitState(stale_oc, BOS_FINISHED); - stale_state = stale_boc->state; + stale_state = ObjWaitState(stale_oc, BOS_FINISHED); HSH_DerefBoc(bo->wrk, stale_oc); stale_boc = NULL; if (stale_state != BOS_FINISHED) {