Please find attached a patch that reworks the busyobj handling code somewhat, and also fixes bug 1068.
Regards, Martin Blix Grydeland -- Martin Blix Grydeland Varnish Software AS
commit 4558d143b781d4f6367e62cf9599d4bf32855633 Author: Martin Blix Grydeland <[email protected]> Date: Fri Dec 2 16:17:09 2011 +0100 Rework the busyobj handling. This patch reworks busyobj handling so that busyobjs are owned by the issuing worker, and the worker should explicitly release it when done with it. A busy objcore only loans a reference to it. This is to fix the current situation where the owning party is murky, and the current code will leak busyobjs in at least one situation. Patch also adds several asserts to help make sure the busyobjs are sound. Fixes: #1068 diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index ff9dcea..f128460 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -985,6 +985,7 @@ void SMP_NewBan(const uint8_t *ban, unsigned len); #define New_BusyObj(wrk) \ do { \ + AZ(wrk->busyobj); \ if (wrk->nbusyobj != NULL) { \ CHECK_OBJ_NOTNULL(wrk->nbusyobj, BUSYOBJ_MAGIC);\ wrk->busyobj = wrk->nbusyobj; \ @@ -998,6 +999,20 @@ void SMP_NewBan(const uint8_t *ban, unsigned len); AZ(wrk->nbusyobj); \ } while (0) +#define Rel_BusyObj(wrk) \ + do { \ + CHECK_OBJ_NOTNULL(wrk->busyobj, BUSYOBJ_MAGIC); \ + if (wrk->obj != NULL && wrk->obj->objcore != NULL) \ + AZ(wrk->obj->objcore->busyobj); \ + if (wrk->objcore != NULL) \ + AZ(wrk->objcore->busyobj); \ + if (wrk->nbusyobj == NULL) \ + wrk->nbusyobj = wrk->busyobj; \ + else \ + FREE_OBJ(wrk->busyobj); \ + wrk->busyobj = NULL; \ + } while (0) + /* * A normal pointer difference is signed, but we never want a negative value * so this little tool will make sure we don't get that. @@ -1052,6 +1067,7 @@ AssertObjBusy(const struct object *o) { AN(o->objcore); AN (o->objcore->flags & OC_F_BUSY); + AN(o->objcore->busyobj); } static inline void diff --git a/bin/varnishd/cache/cache_center.c b/bin/varnishd/cache/cache_center.c index 501293d..12e3203 100644 --- a/bin/varnishd/cache/cache_center.c +++ b/bin/varnishd/cache/cache_center.c @@ -179,8 +179,11 @@ cnt_prepresp(struct sess *sp) CHECK_OBJ_NOTNULL(wrk->obj, OBJECT_MAGIC); CHECK_OBJ_NOTNULL(sp->vcl, VCL_CONF_MAGIC); - if (wrk->busyobj != NULL && wrk->busyobj->do_stream) + if (wrk->busyobj != NULL) { + CHECK_OBJ_NOTNULL(wrk->busyobj, BUSYOBJ_MAGIC); + AN(wrk->busyobj->do_stream); AssertObjCorePassOrBusy(wrk->obj->objcore); + } wrk->res_mode = 0; @@ -248,9 +251,11 @@ cnt_prepresp(struct sess *sp) case VCL_RET_RESTART: if (sp->restarts >= cache_param->max_restarts) break; - if (wrk->busyobj->do_stream) { + if (wrk->busyobj != NULL) { + AN(wrk->busyobj->do_stream); VDI_CloseFd(wrk); HSH_Drop(wrk); + Rel_BusyObj(wrk); } else { (void)HSH_Deref(wrk, NULL, &wrk->obj); } @@ -298,6 +303,7 @@ cnt_deliver(struct sess *sp) wrk = sp->wrk; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); + AZ(sp->wrk->busyobj); sp->director = NULL; sp->restarts = 0; @@ -339,6 +345,7 @@ cnt_done(struct sess *sp) AZ(wrk->obj); AZ(wrk->vbc); + AZ(wrk->busyobj); sp->director = NULL; sp->restarts = 0; @@ -478,6 +485,7 @@ cnt_error(struct sess *sp) wrk->obj->xid = sp->xid; wrk->obj->exp.entered = sp->t_req; } else { + CHECK_OBJ_NOTNULL(wrk->busyobj, BUSYOBJ_MAGIC); /* XXX: Null the headers ? */ } CHECK_OBJ_NOTNULL(wrk->obj, OBJECT_MAGIC); @@ -502,6 +510,7 @@ cnt_error(struct sess *sp) if (sp->handling == VCL_RET_RESTART && sp->restarts < cache_param->max_restarts) { HSH_Drop(wrk); + Rel_BusyObj(wrk); sp->director = NULL; sp->restarts++; sp->step = STP_RECV; @@ -519,6 +528,7 @@ cnt_error(struct sess *sp) sp->err_reason = NULL; http_Setup(wrk->bereq, NULL); sp->step = STP_PREPRESP; + Rel_BusyObj(wrk); return (0); } @@ -644,6 +654,7 @@ cnt_fetch(struct sess *sp) AZ(HSH_Deref(wrk, wrk->objcore, NULL)); wrk->objcore = NULL; } + Rel_BusyObj(wrk); http_Setup(wrk->bereq, NULL); http_Setup(wrk->beresp, NULL); sp->director = NULL; @@ -818,6 +829,7 @@ cnt_fetchbody(struct sess *sp) sp->err_code = 503; sp->step = STP_ERROR; VDI_CloseFd(wrk); + Rel_BusyObj(wrk); return (0); } CHECK_OBJ_NOTNULL(wrk->obj, OBJECT_MAGIC); @@ -886,6 +898,7 @@ cnt_fetchbody(struct sess *sp) if (i) { HSH_Drop(wrk); + Rel_BusyObj(wrk); AZ(wrk->obj); sp->err_code = 503; sp->step = STP_ERROR; @@ -898,6 +911,7 @@ cnt_fetchbody(struct sess *sp) AN(wrk->obj->objcore->ban); HSH_Unbusy(wrk); } + Rel_BusyObj(wrk); wrk->acct_tmp.fetch++; sp->step = STP_PREPRESP; return (0); @@ -971,6 +985,7 @@ cnt_streambody(struct sess *sp) assert(WRW_IsReleased(wrk)); assert(wrk->wrw.ciov == wrk->wrw.siov); (void)HSH_Deref(wrk, NULL, &wrk->obj); + Rel_BusyObj(wrk); http_Setup(wrk->resp, NULL); sp->step = STP_DONE; return (0); @@ -1049,6 +1064,7 @@ cnt_hit(struct sess *sp) CHECK_OBJ_NOTNULL(wrk->obj, OBJECT_MAGIC); CHECK_OBJ_NOTNULL(sp->vcl, VCL_CONF_MAGIC); + AZ(wrk->busyobj); assert(!(wrk->obj->objcore->flags & OC_F_PASS)); @@ -1066,7 +1082,6 @@ cnt_hit(struct sess *sp) /* Drop our object, we won't need it */ (void)HSH_Deref(wrk, NULL, &wrk->obj); wrk->objcore = NULL; - wrk->busyobj = NULL; switch(sp->handling) { case VCL_RET_PASS: @@ -1126,6 +1141,7 @@ cnt_lookup(struct sess *sp) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(sp->vcl, VCL_CONF_MAGIC); + AZ(wrk->busyobj); if (sp->hash_objhead == NULL) { /* Not a waiting list return */ @@ -1263,12 +1279,14 @@ cnt_miss(struct sess *sp) case VCL_RET_ERROR: AZ(HSH_Deref(wrk, wrk->objcore, NULL)); wrk->objcore = NULL; + Rel_BusyObj(wrk); http_Setup(wrk->bereq, NULL); sp->step = STP_ERROR; return (0); case VCL_RET_PASS: AZ(HSH_Deref(wrk, wrk->objcore, NULL)); wrk->objcore = NULL; + Rel_BusyObj(wrk); sp->step = STP_PASS; return (0); case VCL_RET_FETCH: @@ -1278,6 +1296,7 @@ cnt_miss(struct sess *sp) case VCL_RET_RESTART: AZ(HSH_Deref(wrk, wrk->objcore, NULL)); wrk->objcore = NULL; + Rel_BusyObj(wrk); INCOMPL(); default: WRONG("Illegal action in vcl_miss{}"); @@ -1326,6 +1345,7 @@ cnt_pass(struct sess *sp) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(sp->vcl, VCL_CONF_MAGIC); AZ(wrk->obj); + AZ(wrk->busyobj); WS_Reset(wrk->ws, NULL); http_Setup(wrk->bereq, wrk->ws); @@ -1432,6 +1452,7 @@ cnt_recv(struct sess *sp) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(sp->vcl, VCL_CONF_MAGIC); AZ(wrk->obj); + AZ(wrk->busyobj); assert(wrk->wrw.ciov == wrk->wrw.siov); /* By default we use the first backend */ diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 1f1795c..de99781 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -626,8 +626,6 @@ HSH_Unbusy(struct worker *wrk) VTAILQ_REMOVE(&oh->objcs, oc, list); VTAILQ_INSERT_HEAD(&oh->objcs, oc, list); oc->flags &= ~OC_F_BUSY; - AZ(wrk->nbusyobj); - wrk->nbusyobj = oc->busyobj; oc->busyobj = NULL; if (oh->waitinglist != NULL) hsh_rush(oh); @@ -716,15 +714,9 @@ HSH_Deref(struct worker *w, struct objcore *oc, struct object **oo) BAN_DestroyObj(oc); AZ(oc->ban); - if (oc->flags & OC_F_BUSY) { + if (oc->flags & OC_F_BUSY) CHECK_OBJ_NOTNULL(oc->busyobj, BUSYOBJ_MAGIC); - if (w->nbusyobj == NULL) - w->nbusyobj = oc->busyobj; - else - FREE_OBJ(oc->busyobj); - oc->busyobj = NULL; - } - AZ(oc->busyobj); + oc->busyobj = NULL; if (oc->methods != NULL) { oc_freeobj(oc); diff --git a/bin/varnishtest/tests/r01068.vtc b/bin/varnishtest/tests/r01068.vtc new file mode 100644 index 0000000..c055d19 --- /dev/null +++ b/bin/varnishtest/tests/r01068.vtc @@ -0,0 +1,24 @@ +varnishtest "Bug 1068 restart on hit in vcl_deliver causes segfault" + +server s1 { + rxreq + txresp +} -start + +varnish v1 -vcl+backend { + sub vcl_deliver { + if (req.http.x-restart && req.restarts == 0) { + return (restart); + } + } +} -start + +client c1 { + txreq + rxresp + expect resp.status == 200 + + txreq -hdr "x-restart: true" + rxresp + expect resp.status == 200 +} -run
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
