[4.1] 713be80 Close race condition on HTTP1 session step counter

[Martin Blix Grydeland]

commit 713be801409a3fabd68f450a32c0f49b263a2f3d
Author: Martin Blix Grydeland <martin at varnish-software.com>
Date:   Thu Dec 8 17:43:23 2016 +0100

    Close race condition on HTTP1 session step counter
    
    When going on a waitinglist, the HTTP1 step counter was set to
    S_STP_H1BUSY to allow conditions to be checked on return specific to
    having been on a waitinglist. Though the step counter was set after
    entering the waitinglist, and if the session was rescheduled
    immediately on another thread a race opened which would mess up the
    state handling.
    
    Fix this by elliminating the S_STP_H1BUSY step, and having condition
    checks on req->hash_objhead in the S_STP_H1PROC state to handle the
    waitinglist return specific checks.
    
    The panic output has been changed to include the req->hash_objhead
    pointer if set. This exposes the waitinglist condition in the panic
    output which would otherwise be hidden by the removal of the
    S_STP_H1BUSY step.
    
    Fixes: #2117

diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c
index 589f9f5..de5ce0c 100644
--- a/bin/varnishd/cache/cache_panic.c
+++ b/bin/varnishd/cache/cache_panic.c
@@ -345,6 +345,8 @@ pan_req(struct vsb *vsb, const struct req *req)
 		VSB_printf(vsb, "step = %s,\n", stp);
 	else
 		VSB_printf(vsb, "step = 0x%x,\n", req->req_step);
+	if (req->hash_objhead)
+		VSB_printf(vsb, "hash_objhead = %p\n", req->hash_objhead);
 
 	VSB_printf(vsb, "req_body = %s,\n",
 	    reqbody_status_2str(req->req_body_status));
diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
index ad9b069..9b4313e 100644
--- a/bin/varnishd/http1/cache_http1_fsm.c
+++ b/bin/varnishd/http1/cache_http1_fsm.c
@@ -247,27 +247,22 @@ HTTP1_Session(struct worker *wrk, struct req *req)
 			req->req_step = R_STP_RECV;
 			sp->sess_step = S_STP_H1PROC;
 			break;
-		case S_STP_H1BUSY:
-			/*
-			 * Return from waitinglist.
-			 * Check to see if the remote has left.
-			 */
-			if (VTCP_check_hup(sp->fd)) {
-				AN(req->hash_objhead);
+		case S_STP_H1PROC:
+			req->transport = &http1_transport;
+			req->task.func = SES_Proto_Req;
+			req->task.priv = req;
+			if (req->hash_objhead && VTCP_check_hup(sp->fd)) {
+				/* Return from waitinglist and the remote
+				   has left. */
 				(void)HSH_DerefObjHead(wrk, &req->hash_objhead);
 				AZ(req->hash_objhead);
 				SES_Close(sp, SC_REM_CLOSE);
 				AN(Req_Cleanup(sp, wrk, req));
 				return;
 			}
-			sp->sess_step = S_STP_H1PROC;
-			break;
-		case S_STP_H1PROC:
-			req->transport = &http1_transport;
-			req->task.func = SES_Proto_Req;
-			req->task.priv = req;
 			if (CNT_Request(wrk, req) == REQ_FSM_DISEMBARK) {
-				sp->sess_step = S_STP_H1BUSY;
+				/* Have been placed on waitinglist. Any
+				   changes to req and sp are unsafe. */
 				return;
 			}
 			req->transport = NULL;
diff --git a/include/tbl/steps.h b/include/tbl/steps.h
index 4756d44..bd6c1eb 100644
--- a/include/tbl/steps.h
+++ b/include/tbl/steps.h
@@ -34,7 +34,6 @@
 SESS_STEP(h1newsess,	H1NEWSESS)
 SESS_STEP(h1newreq,	H1NEWREQ)
 SESS_STEP(h1proc,	H1PROC)
-SESS_STEP(h1busy,	H1BUSY)
 SESS_STEP(h1cleanup,	H1CLEANUP)
 SESS_STEP(h1_last,	H1_LAST)