From 730c2121ca63bef774a5097b07dc01472169da6e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 9 Dec 2024 18:11:40 +0100 Subject: [PATCH 1/2] vrt_vcl: Simplify director search loop I had to stop and think twice to realize that I was looking at a simple search loop extracting the result inside its critical section. Isn't the real problem a TOCTOU race between VRT_LookupDirector() and VRT_Assign_Backend()? --- bin/varnishd/cache/cache_vrt_vcl.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c index 82e6e10a5d6..e9fc456d4f8 100644 --- a/bin/varnishd/cache/cache_vrt_vcl.c +++ b/bin/varnishd/cache/cache_vrt_vcl.c @@ -373,7 +373,7 @@ VRT_LookupDirector(VRT_CTX, VCL_STRING name) { struct vcl *vcl; struct vcldir *vdir; - VCL_BACKEND dd, d = NULL; + VCL_BACKEND dir = NULL; CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); AN(name); @@ -386,15 +386,22 @@ VRT_LookupDirector(VRT_CTX, VCL_STRING name) Lck_Lock(&vcl_mtx); VTAILQ_FOREACH(vdir, &vcl->director_list, list) { - dd = vdir->dir; - if (strcmp(dd->vcl_name, name)) - continue; - d = dd; - break; + if (!strcmp(vdir->dir->vcl_name, name)) { + dir = vdir->dir; + break; + } } Lck_Unlock(&vcl_mtx); - return (d); + /* XXX: There is a race here between the supposedly incoming call + * to VRT_Assign_Backend() and the next vcldir_deref() that could + * even originate from the former. + * + * Maybe the VRT_CTX could keep a ref of the last director lookup + * that would linger until the end of vcl_init. + */ + + return (dir); } /*--------------------------------------------------------------------*/ From 857571817d7ee2577578d2492dcf7160e197da96 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 9 Dec 2024 18:43:01 +0100 Subject: [PATCH 2/2] vrt_vcl: Avoid retirement of assigned director Consider the following VCL statement: set bereq.backend = bereq.backend; The director's refcount will be decremented before it is incremented. If that bereq.backend was holding the last reference of a dynamic backend, the backend is retired and grab a reference afterwards. While this scenario is highly unlikely, we cannot discard the risk of assigning a director to a destination that already held a reference to the same director. --- bin/varnishd/cache/cache_vrt_vcl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c index e9fc456d4f8..f7339f9addd 100644 --- a/bin/varnishd/cache/cache_vrt_vcl.c +++ b/bin/varnishd/cache/cache_vrt_vcl.c @@ -336,6 +336,8 @@ VRT_Assign_Backend(VCL_BACKEND *dst, VCL_BACKEND src) AN(dst); CHECK_OBJ_ORNULL((*dst), DIRECTOR_MAGIC); CHECK_OBJ_ORNULL(src, DIRECTOR_MAGIC); + if (*dst == src) + return; if (*dst != NULL) { vdir = (*dst)->vdir; CHECK_OBJ_NOTNULL(vdir, VCLDIR_MAGIC); @@ -394,8 +396,7 @@ VRT_LookupDirector(VRT_CTX, VCL_STRING name) Lck_Unlock(&vcl_mtx); /* XXX: There is a race here between the supposedly incoming call - * to VRT_Assign_Backend() and the next vcldir_deref() that could - * even originate from the former. + * to VRT_Assign_Backend() and the next vcldir_deref() call. * * Maybe the VRT_CTX could keep a ref of the last director lookup * that would linger until the end of vcl_init.