Hi, here's the first set of patches to address #1506. These do not cover Range-Requests yet, but these should be easy to address once we got this first set in.
PATCH 1/4 is a prerequisite for the next patch. It contains what is required to test for premature shutdown of client connections by Varnish, handling for bodies longer than the available buffer in varnishtest plus a reduction of the number of read calls. PATCH 2/4 is the actual first fix for #1506 PATCH 3/4 adds an assertion to VBO_DerefBusyObj which I found helpful during testing. *pbo = NULL is delayed until after the test for the case pbo = &oc->busyobj PATCH 4/4 addresses races I have observed with objcore->busyobj bering set to NULL while being accessed Reviews appreciated Thanks, Nils
>From 583eb09cfbe174542d049e071f1a39237084c9f4 Mon Sep 17 00:00:00 2001 From: Nils Goroll <[email protected]> Date: Thu, 19 Jun 2014 08:10:21 +0200 Subject: [PATCH 1/4] http_rxchar and http_swallow_body improvements, some additional assertions http_rxchar: * limit data to be read to the available space * for n == -1, read up to available space * return 0 upon poll failure * return bytes read otherwise http_swallow_body: * allow to test for short client reads * handle bodies which do not fit into rxbuf * set hp->bodyl to the number of bytes actually read * use less http_rxchar calls for rxeof --- bin/varnishtest/vtc_http.c | 61 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/bin/varnishtest/vtc_http.c b/bin/varnishtest/vtc_http.c index 28b7340..c89b1fb 100644 --- a/bin/varnishtest/vtc_http.c +++ b/bin/varnishtest/vtc_http.c @@ -349,28 +349,39 @@ http_splitheader(struct http *hp, int req) /********************************************************************** - * Receive another character + * Receive data */ static int http_rxchar(struct http *hp, int n, int eof) { int i; + int l = 0; struct pollfd pfd[1]; + int avail = hp->nrxbuf - hp->prxbuf - 1; + + if (n == -1 || n > avail) + n = avail; while (n > 0) { pfd[0].fd = hp->fd; pfd[0].events = POLLIN; pfd[0].revents = 0; i = poll(pfd, 1, hp->timeout); - if (i == 0) + if (i == 0) { vtc_log(hp->vl, hp->fatal, "HTTP rx timeout (fd:%d %u ms)", hp->fd, hp->timeout); - if (i < 0) + return (0); + } + + if (i < 0) { vtc_log(hp->vl, hp->fatal, "HTTP rx failed (fd:%d poll: %s)", hp->fd, strerror(errno)); + return (0); + } + assert(i > 0); assert(hp->prxbuf + n < hp->nrxbuf); i = read(hp->fd, hp->rxbuf + hp->prxbuf, n); @@ -378,8 +389,10 @@ http_rxchar(struct http *hp, int n, int eof) vtc_log(hp->vl, 4, "HTTP rx poll (fd:%d revents: %x n=%d, i=%d)", hp->fd, pfd[0].revents, n, i); + if (i == 0 && eof) - return (i); + return (l); + if (i == 0) vtc_log(hp->vl, hp->fatal, "HTTP rx EOF (fd:%d read: %s)", @@ -388,11 +401,16 @@ http_rxchar(struct http *hp, int n, int eof) vtc_log(hp->vl, hp->fatal, "HTTP rx failed (fd:%d read: %s)", hp->fd, strerror(errno)); + + if (i <= 0) + return (l); + hp->prxbuf += i; hp->rxbuf[hp->prxbuf] = '\0'; n -= i; + l += i; } - return (1); + return (l); } static int @@ -403,7 +421,7 @@ http_rxchunk(struct http *hp) l = hp->prxbuf; do - (void)http_rxchar(hp, 1, 0); + assert(http_rxchar(hp, 1, 0) == 1); while (hp->rxbuf[hp->prxbuf - 1] != '\n'); vtc_dump(hp->vl, 4, "len", hp->rxbuf + l, -1); i = strtoul(hp->rxbuf + l, &q, 16); @@ -417,12 +435,12 @@ http_rxchunk(struct http *hp) assert(*q == '\0' || vct_islws(*q)); hp->prxbuf = l; if (i > 0) { - (void)http_rxchar(hp, i, 0); + i = http_rxchar(hp, i, 0); vtc_dump(hp->vl, 4, "chunk", hp->rxbuf + l, i); } l = hp->prxbuf; - (void)http_rxchar(hp, 2, 0); + assert(http_rxchar(hp, 2, 0) == 2); if(!vct_iscrlf(hp->rxbuf + l)) vtc_log(hp->vl, hp->fatal, "Wrong chunk tail[0] = %02x", @@ -438,23 +456,32 @@ http_rxchunk(struct http *hp) /********************************************************************** * Swallow a HTTP message body + * + * if the body does not fit into rxbuf, bp->body will point to the last + * received buffer */ static void http_swallow_body(struct http *hp, char * const *hh, int body) { char *p; - int i, l, ll; + int i, l, ll, oprxbuf; ll = 0; p = http_find_header(hh, "content-length"); if (p != NULL) { hp->body = hp->rxbuf + hp->prxbuf; l = strtoul(p, NULL, 0); - (void)http_rxchar(hp, l, 0); - vtc_dump(hp->vl, 4, "body", hp->body, l); - hp->bodyl = l; - sprintf(hp->bodylen, "%d", l); + oprxbuf = hp->prxbuf; + do { + hp->prxbuf = oprxbuf; + i = http_rxchar(hp, l, 1); + l -= i; + ll += i; + vtc_dump(hp->vl, 4, "body", hp->body, ll); + } while (i > 0 && l > 0); + hp->bodyl = ll; + sprintf(hp->bodylen, "%d", ll); return; } p = http_find_header(hh, "transfer-encoding"); @@ -469,11 +496,13 @@ http_swallow_body(struct http *hp, char * const *hh, int body) } if (body) { hp->body = hp->rxbuf + hp->prxbuf; + oprxbuf = hp->prxbuf; do { - i = http_rxchar(hp, 1, 1); + hp->prxbuf = oprxbuf; + i = http_rxchar(hp, -1, 1); ll += i; + vtc_dump(hp->vl, 4, "rxeof", hp->body, ll); } while (i > 0); - vtc_dump(hp->vl, 4, "rxeof", hp->body, ll); } hp->bodyl = ll; sprintf(hp->bodylen, "%d", ll); @@ -1057,7 +1086,7 @@ cmd_http_timeout(CMD_ARGS) } /********************************************************************** - * expect other end to close (server only) + * expect other end to close */ static void -- 2.0.0
>From a6a516a9e93b8752f8a1048a534626cf6b7ea0d1 Mon Sep 17 00:00:00 2001 From: Nils Goroll <[email protected]> Date: Tue, 17 Jun 2014 20:08:38 +0200 Subject: [PATCH 2/4] Announce the Content-Length from the backend to clients when streaming. Close the client connection when ww failed to send the what the backend announced. Fixes #1506 partly (not for range requests yet) --- bin/varnishd/cache/cache.h | 1 + bin/varnishd/cache/cache_busyobj.c | 3 ++ bin/varnishd/cache/cache_http1_deliver.c | 26 +++++++++++ bin/varnishd/cache/cache_http1_fetch.c | 7 ++- bin/varnishtest/tests/t00000.vtc | 75 ++++++++++++++++++++++++++++++++ include/tbl/sess_close.h | 1 + 6 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 bin/varnishtest/tests/t00000.vtc diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index 554dc8f..d77258e 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -560,6 +560,7 @@ struct busyobj { struct pool_task fetch_task; char *h_content_length; + ssize_t adv_len; #define BO_FLAG(l, r, w, d) unsigned l:1; #include "tbl/bo_flags.h" diff --git a/bin/varnishd/cache/cache_busyobj.c b/bin/varnishd/cache/cache_busyobj.c index 45e2845..ba5d983 100644 --- a/bin/varnishd/cache/cache_busyobj.c +++ b/bin/varnishd/cache/cache_busyobj.c @@ -150,6 +150,9 @@ VBO_GetBusyObj(struct worker *wrk, const struct req *req) bo->t_first = bo->t_prev = NAN; + bo->h_content_length = NULL; + bo->adv_len = 0; + return (bo); } diff --git a/bin/varnishd/cache/cache_http1_deliver.c b/bin/varnishd/cache/cache_http1_deliver.c index 1bdcef7..5c7b6fe 100644 --- a/bin/varnishd/cache/cache_http1_deliver.c +++ b/bin/varnishd/cache/cache_http1_deliver.c @@ -256,6 +256,19 @@ V1D_Deliver(struct req *req) http_PrintfHeader(req->resp, "Content-Length: %zd", req->obj->len); } + } else if (req->obj->objcore->busyobj->h_content_length && + ! req->obj->changed_gzip) { + /* + * streaming a backend object with C-L - trust the backend - we + * will re-check the written bytes later and close with + * SC_RESP_SHORT if they don't match + */ + req->res_mode |= RES_LEN; + http_Unset(req->resp, H_Content_Length); + http_PrintfHeader(req->resp, + "Content-Length: %zd", req->obj->objcore->busyobj->adv_len); + http_PrintfHeader(req->resp, + "Content-Length-DEBUG-XXX: %zd", req->obj->len); } if (req->esi_level > 0) { @@ -355,6 +368,19 @@ V1D_Deliver(struct req *req) !(req->res_mode & RES_ESI_CHILD)) WRW_EndChunk(req->wrk); + /* close connection if we announced a wrong c-l for unchunked writes */ + if (req->wantbody && + req->res_mode & RES_LEN && + req->sp->fd >= 0) { + if (req->resp->status == 206) { + if (req->resp_bodybytes != (req->range_high - req->range_low)) + SES_Close(req->sp, SC_RESP_SHORT); + } else { + if (req->resp_bodybytes != req->obj->len) + SES_Close(req->sp, SC_RESP_SHORT); + } + } + if ((V1D_FlushReleaseAcct(req) || ois != OIS_DONE) && req->sp->fd >= 0) SES_Close(req->sp, SC_REM_CLOSE); } diff --git a/bin/varnishd/cache/cache_http1_fetch.c b/bin/varnishd/cache/cache_http1_fetch.c index b6d72dd..05341e9 100644 --- a/bin/varnishd/cache/cache_http1_fetch.c +++ b/bin/varnishd/cache/cache_http1_fetch.c @@ -167,7 +167,6 @@ ssize_t V1F_Setup_Fetch(struct busyobj *bo) { struct http_conn *htc; - ssize_t cl; CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC); htc = &bo->htc; @@ -179,9 +178,9 @@ V1F_Setup_Fetch(struct busyobj *bo) VFP_Push(bo, v1f_pull_eof, 0); return(-1); case BS_LENGTH: - cl = vbf_fetch_number(bo->h_content_length, 10); - VFP_Push(bo, v1f_pull_straight, cl); - return (cl); + bo->adv_len = vbf_fetch_number(bo->h_content_length, 10); + VFP_Push(bo, v1f_pull_straight, bo->adv_len); + return (bo->adv_len); case BS_CHUNKED: VFP_Push(bo, v1f_pull_chunked, -1); return (-1); diff --git a/bin/varnishtest/tests/t00000.vtc b/bin/varnishtest/tests/t00000.vtc new file mode 100644 index 0000000..5a104e7 --- /dev/null +++ b/bin/varnishtest/tests/t00000.vtc @@ -0,0 +1,75 @@ +varnishtest "streaming and Content-Length" + +server s1 { + rxreq + expect req.url == "/ok" + txresp -bodylen 1000000 + + rxreq + expect req.url == "/short" + delay 1 + txresp -hdr "Content-Length: 1000000" -nolen -bodylen 500000 + + accept + + rxreq + expect req.url == "/short" + txresp -bodylen 1000000 +} -start + +varnish v1 \ + -arg "-smalloc,10m" \ + -vcl+backend { +} -start + +logexpect l1 -v v1 { + expect * 1002 Fetch_Body "length stream$" +} -start + +client c1 { + # streaming the busy obj + txreq -url /ok + rxresp + expect resp.status == 200 + expect resp.http.Content-Length == 1000000 + expect resp.bodylen == 1000000 + + # cached + txreq -url /ok + rxresp + expect resp.status == 200 + expect resp.http.Content-Length == 1000000 + expect resp.bodylen == 1000000 +} -run + +logexpect l1 -wait + +logexpect l2 -v v1 { + expect * * FetchError "straight insufficient bytes$" +} -start + +# should see the close-on-short +client c2 { + # streaming with close on short + txreq -url /short + rxresp + expect_close + expect resp.status == 200 + expect resp.http.Content-Length == 1000000 + expect resp.bodylen == 500000 +} -start + +logexpect l2 -wait + +client c2 -wait + +# check the fetch error is not cached +client c3 { + txreq -url /short + rxresp + expect resp.status == 200 + expect resp.http.Content-Length == 1000000 + expect resp.bodylen == 1000000 +} -run + +server s1 -wait diff --git a/include/tbl/sess_close.h b/include/tbl/sess_close.h index 5b77eba..4095538 100644 --- a/include/tbl/sess_close.h +++ b/include/tbl/sess_close.h @@ -40,6 +40,7 @@ SESS_CLOSE(TX_PIPE, "Piped transaction") SESS_CLOSE(TX_ERROR, "Error transaction") SESS_CLOSE(TX_EOF, "EOF transmission") SESS_CLOSE(RESP_CLOSE, "Backend/VCL requested close") +SESS_CLOSE(RESP_SHORT, "Backend short read") SESS_CLOSE(OVERLOAD, "Out of some resource") SESS_CLOSE(SESS_PIPE_OVERFLOW, "Session pipe overflow") -- 2.0.0
>From d7397782fed6748e795288a7862a75d1434e30e7 Mon Sep 17 00:00:00 2001 From: Nils Goroll <[email protected]> Date: Thu, 26 Jun 2014 08:10:07 +0200 Subject: [PATCH 3/4] If the busy object has a fetch_objcore, the latter should reference the former --- bin/varnishd/cache/cache_busyobj.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/cache/cache_busyobj.c b/bin/varnishd/cache/cache_busyobj.c index ba5d983..ad5e108 100644 --- a/bin/varnishd/cache/cache_busyobj.c +++ b/bin/varnishd/cache/cache_busyobj.c @@ -166,7 +166,6 @@ VBO_DerefBusyObj(struct worker *wrk, struct busyobj **pbo) CHECK_OBJ_ORNULL(wrk, WORKER_MAGIC); AN(pbo); bo = *pbo; - *pbo = NULL; CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC); CHECK_OBJ_ORNULL(bo->fetch_objcore, OBJCORE_MAGIC); CHECK_OBJ_ORNULL(bo->fetch_obj, OBJECT_MAGIC); @@ -174,13 +173,17 @@ VBO_DerefBusyObj(struct worker *wrk, struct busyobj **pbo) oc = bo->fetch_objcore; CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); CHECK_OBJ_NOTNULL(oc->objhead, OBJHEAD_MAGIC); + if (oc->busyobj != NULL) + assert(oc->busyobj == bo); Lck_Lock(&oc->objhead->mtx); assert(bo->refcount > 0); + *pbo = NULL; r = --bo->refcount; Lck_Unlock(&oc->objhead->mtx); } else { Lck_Lock(&bo->mtx); assert(bo->refcount > 0); + *pbo = NULL; r = --bo->refcount; Lck_Unlock(&bo->mtx); } -- 2.0.0
>From 81edbd36e3d555def9865cc10346b952a607e92b Mon Sep 17 00:00:00 2001 From: Nils Goroll <[email protected]> Date: Thu, 26 Jun 2014 08:21:24 +0200 Subject: [PATCH 4/4] Hold a refernce to the busy object while accessing its members. This fixes a couple of races having become apparent when testing streaming --- bin/varnishd/cache/cache.h | 3 ++ bin/varnishd/cache/cache_http1_deliver.c | 60 +++++++++++++++++++++----------- bin/varnishd/hash/hash_slinger.h | 1 - 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index d77258e..4a69feb 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -1272,6 +1272,9 @@ struct storage *STV_alloc_transient(size_t size); void SMP_Init(void); void SMP_Ready(void); +/* cache_hash.c */ +struct busyobj *HSH_RefBusy(const struct objcore *oc); + /* * 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. diff --git a/bin/varnishd/cache/cache_http1_deliver.c b/bin/varnishd/cache/cache_http1_deliver.c index 5c7b6fe..3a04143 100644 --- a/bin/varnishd/cache/cache_http1_deliver.c +++ b/bin/varnishd/cache/cache_http1_deliver.c @@ -90,16 +90,24 @@ static void v1d_dorange(struct req *req, const char *r) { ssize_t len, low, high, has_low; + struct busyobj *bo; CHECK_OBJ_NOTNULL(req, REQ_MAGIC); CHECK_OBJ_NOTNULL(req->obj, OBJECT_MAGIC); assert(http_GetStatus(req->obj->http) == 200); /* We must snapshot the length if we're streaming from the backend */ - if (req->obj->objcore->busyobj != NULL) - len = VBO_waitlen(req->obj->objcore->busyobj, -1); + if (req->obj->objcore->busyobj) + bo = HSH_RefBusy(req->obj->objcore); else + bo = NULL; + + if (bo != NULL) { + len = VBO_waitlen(bo, -1); + VBO_DerefBusyObj(req->wrk, &bo); + } else { len = req->obj->len; + } if (strncmp(r, "bytes=", 6)) return; @@ -232,6 +240,7 @@ V1D_Deliver(struct req *req) { char *r; enum objiter_status ois; + struct busyobj *bo; CHECK_OBJ_NOTNULL(req, REQ_MAGIC); CHECK_OBJ_NOTNULL(req->obj, OBJECT_MAGIC); @@ -247,28 +256,37 @@ V1D_Deliver(struct req *req) req->res_mode &= ~RES_LEN; http_Unset(req->resp, H_Content_Length); req->wantbody = 0; - } else if (req->obj->objcore->busyobj == NULL) { - /* XXX: Not happy with this convoluted test */ - req->res_mode |= RES_LEN; - if (!(req->obj->objcore->flags & OC_F_PASS) || - req->obj->len != 0) { + } else { + if (req->obj->objcore->busyobj) + bo = HSH_RefBusy(req->obj->objcore); + else + bo = NULL; + + if (bo == NULL) { + /* XXX: Not happy with this convoluted test */ + req->res_mode |= RES_LEN; + if (!(req->obj->objcore->flags & OC_F_PASS) || + req->obj->len != 0) { + http_Unset(req->resp, H_Content_Length); + http_PrintfHeader(req->resp, + "Content-Length: %zd", req->obj->len); + } + } else if (bo->h_content_length && ! req->obj->changed_gzip) { + /* + * streaming a backend object with C-L - trust the + * backend - we will re-check the written bytes later + * and close with SC_RESP_SHORT if they don't match + */ + req->res_mode |= RES_LEN; http_Unset(req->resp, H_Content_Length); http_PrintfHeader(req->resp, - "Content-Length: %zd", req->obj->len); + "Content-Length: %zd", bo->adv_len); + http_PrintfHeader(req->resp, + "Content-Length-DEBUG-XXX: %zd", req->obj->len); } - } else if (req->obj->objcore->busyobj->h_content_length && - ! req->obj->changed_gzip) { - /* - * streaming a backend object with C-L - trust the backend - we - * will re-check the written bytes later and close with - * SC_RESP_SHORT if they don't match - */ - req->res_mode |= RES_LEN; - http_Unset(req->resp, H_Content_Length); - http_PrintfHeader(req->resp, - "Content-Length: %zd", req->obj->objcore->busyobj->adv_len); - http_PrintfHeader(req->resp, - "Content-Length-DEBUG-XXX: %zd", req->obj->len); + + if (bo != NULL) + VBO_DerefBusyObj(req->wrk, &bo); } if (req->esi_level > 0) { diff --git a/bin/varnishd/hash/hash_slinger.h b/bin/varnishd/hash/hash_slinger.h index 3c6f815..df04cde 100644 --- a/bin/varnishd/hash/hash_slinger.h +++ b/bin/varnishd/hash/hash_slinger.h @@ -73,7 +73,6 @@ void HSH_Insert(struct worker *, const void *hash, struct objcore *); void HSH_Purge(struct worker *, struct objhead *, double ttl, double grace, double keep); void HSH_config(const char *h_arg); -struct busyobj *HSH_RefBusy(const struct objcore *oc); struct objcore *HSH_Private(struct worker *wrk); struct objcore *HSH_NewObjCore(struct worker *wrk); -- 2.0.0
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
