[6.2] dd33a9251 Add Session Attribute workspace overflow handling
Martin Blix Grydeland
martin at varnish-software.com
Tue Feb 4 10:03:08 UTC 2020
commit dd33a92513f9339c7e3cd977ff0aae56688c13fe
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Tue Nov 26 14:56:24 2019 +0100
Add Session Attribute workspace overflow handling
Notes:
* for the acceptor, I think it makes sense to keep AN assertion (pun!)
because varnish is not viable if the session workspace is too small
to even hold the attributes initialized in the acceptor.
If this was an issue, we should rather revisit the minimum values for
the session workspace
* for h1 and h2 session setup, I have used XXXAN() because I am not sure
how we should best handle allocation failures.
* The relevant bit, for now, is the proxy code which may allocate
arbitrarily long TLV attributes, so this is the code for which we now
actually handle errors and test that we do
On the vtc: I added the test to o00005.vtc because there existed a
previous overflow test from 267504b8143bdb8ef96240455a3aa788f96b579b,
but that only tested for the one case of a WS overflow which was already
handled.
Fixes varnishcache/varnish-cache#3145
(cherry picked from commit 287dc4a6745c374e0b229bfa861d664989a3a9e8)
diff --git a/bin/varnishd/cache/cache_acceptor.c b/bin/varnishd/cache/cache_acceptor.c
index 8300d1b5d..ee0b6063e 100644
--- a/bin/varnishd/cache/cache_acceptor.c
+++ b/bin/varnishd/cache/cache_acceptor.c
@@ -317,17 +317,17 @@ vca_mk_tcp(const struct wrk_accept *wa,
struct sockaddr_storage ss;
socklen_t sl;
- SES_Reserve_remote_addr(sp, &sa);
+ AN(SES_Reserve_remote_addr(sp, &sa));
AN(VSA_Build(sa, &wa->acceptaddr, wa->acceptaddrlen));
sp->sattr[SA_CLIENT_ADDR] = sp->sattr[SA_REMOTE_ADDR];
VTCP_name(sa, raddr, VTCP_ADDRBUFSIZE, rport, VTCP_PORTBUFSIZE);
- SES_Set_String_Attr(sp, SA_CLIENT_IP, raddr);
- SES_Set_String_Attr(sp, SA_CLIENT_PORT, rport);
+ AN(SES_Set_String_Attr(sp, SA_CLIENT_IP, raddr));
+ AN(SES_Set_String_Attr(sp, SA_CLIENT_PORT, rport));
sl = sizeof ss;
AZ(getsockname(sp->fd, (void*)&ss, &sl));
- SES_Reserve_local_addr(sp, &sa);
+ AN(SES_Reserve_local_addr(sp, &sa));
AN(VSA_Build(sa, &ss, sl));
sp->sattr[SA_SERVER_ADDR] = sp->sattr[SA_LOCAL_ADDR];
VTCP_name(sa, laddr, VTCP_ADDRBUFSIZE, lport, VTCP_PORTBUFSIZE);
@@ -340,13 +340,13 @@ vca_mk_uds(struct wrk_accept *wa, struct sess *sp, char *laddr, char *lport,
struct suckaddr *sa;
(void) wa;
- SES_Reserve_remote_addr(sp, &sa);
+ AN(SES_Reserve_remote_addr(sp, &sa));
AZ(SES_Set_remote_addr(sp, bogo_ip));
sp->sattr[SA_CLIENT_ADDR] = sp->sattr[SA_REMOTE_ADDR];
sp->sattr[SA_LOCAL_ADDR] = sp->sattr[SA_REMOTE_ADDR];
sp->sattr[SA_SERVER_ADDR] = sp->sattr[SA_REMOTE_ADDR];
- SES_Set_String_Attr(sp, SA_CLIENT_IP, "0.0.0.0");
- SES_Set_String_Attr(sp, SA_CLIENT_PORT, "0");
+ AN(SES_Set_String_Attr(sp, SA_CLIENT_IP, "0.0.0.0"));
+ AN(SES_Set_String_Attr(sp, SA_CLIENT_PORT, "0"));
strcpy(laddr, "0.0.0.0");
strcpy(raddr, "0.0.0.0");
diff --git a/bin/varnishd/cache/cache_session.c b/bin/varnishd/cache/cache_session.c
index 8c9e2d7c9..c9a0f5180 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -103,8 +103,8 @@ ses_set_attr(const struct sess *sp, enum sess_attr a, const void *src, int sz)
return (0);
}
-static void
-ses_reserve_attr(struct sess *sp, enum sess_attr a, void **dst, int sz)
+static int
+ses_res_attr(struct sess *sp, enum sess_attr a, void **dst, int sz)
{
ssize_t o;
@@ -113,12 +113,14 @@ ses_reserve_attr(struct sess *sp, enum sess_attr a, void **dst, int sz)
assert(sz >= 0);
AN(dst);
o = WS_ReserveSize(sp->ws, sz);
- assert(o >= sz);
+ if (o < sz)
+ return (0);
*dst = sp->ws->f;
o = sp->ws->f - sp->ws->s;
WS_Release(sp->ws, sz);
assert(o >= 0 && o <= 0xffff);
sp->sattr[a] = (uint16_t)o;
+ return (1);
}
#define SESS_ATTR(UP, low, typ, len) \
@@ -134,16 +136,16 @@ ses_reserve_attr(struct sess *sp, enum sess_attr a, void **dst, int sz)
return (ses_get_attr(sp, SA_##UP, (void**)dst)); \
} \
\
- void \
+ int \
SES_Reserve_##low(struct sess *sp, typ **dst) \
{ \
- assert(len >= 0); \
- ses_reserve_attr(sp, SA_##UP, (void**)dst, len); \
+ assert(len > 0); \
+ return (ses_res_attr(sp, SA_##UP, (void**)dst, len)); \
}
#include "tbl/sess_attr.h"
-void
+int
SES_Set_String_Attr(struct sess *sp, enum sess_attr a, const char *src)
{
void *q;
@@ -157,8 +159,10 @@ SES_Set_String_Attr(struct sess *sp, enum sess_attr a, const char *src)
default: WRONG("wrong sess_attr");
}
- ses_reserve_attr(sp, a, &q, strlen(src) + 1);
+ if (! ses_res_attr(sp, a, &q, strlen(src) + 1))
+ return (0);
strcpy(q, src);
+ return (1);
}
const char *
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index f5c4bf656..4d6ce1022 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -378,9 +378,9 @@ enum htc_status_e HTC_RxStuff(struct http_conn *, htc_complete_f *,
#define SESS_ATTR(UP, low, typ, len) \
int SES_Set_##low(const struct sess *sp, const typ *src); \
- void SES_Reserve_##low(struct sess *sp, typ **dst);
+ int SES_Reserve_##low(struct sess *sp, typ **dst);
#include "tbl/sess_attr.h"
-void SES_Set_String_Attr(struct sess *sp, enum sess_attr a, const char *src);
+int SES_Set_String_Attr(struct sess *sp, enum sess_attr a, const char *src);
enum htc_status_e {
diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
index 4f6e8efd1..2b937ec86 100644
--- a/bin/varnishd/http1/cache_http1_fsm.c
+++ b/bin/varnishd/http1/cache_http1_fsm.c
@@ -112,7 +112,7 @@ http1_new_session(struct worker *wrk, void *arg)
CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
HTC_RxInit(req->htc, req->ws);
- SES_Reserve_proto_priv(sp, &u);
+ XXXAN(SES_Reserve_proto_priv(sp, &u));
http1_setstate(sp, H1NEWREQ);
wrk->task.func = http1_req;
wrk->task.priv = req;
diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c
index efd560db6..788879bbf 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/bin/varnishd/http2/cache_http2_session.c
@@ -101,7 +101,7 @@ h2_init_sess(const struct worker *wrk, struct sess *sp,
if (SES_Get_proto_priv(sp, &up)) {
/* Already reserved if we came via H1 */
- SES_Reserve_proto_priv(sp, &up);
+ XXXAN(SES_Reserve_proto_priv(sp, &up));
*up = 0;
}
if (*up == 0) {
@@ -130,7 +130,7 @@ h2_init_sess(const struct worker *wrk, struct sess *sp,
AZ(VHT_Init(h2->dectbl,
h2->local_settings.header_table_size));
- SES_Reserve_proto_priv(sp, &up);
+ XXXAN(SES_Reserve_proto_priv(sp, &up));
*up = (uintptr_t)h2;
}
AN(up);
diff --git a/bin/varnishd/proxy/cache_proxy_proto.c b/bin/varnishd/proxy/cache_proxy_proto.c
index 86fc2f97d..f5fecad02 100644
--- a/bin/varnishd/proxy/cache_proxy_proto.c
+++ b/bin/varnishd/proxy/cache_proxy_proto.c
@@ -49,6 +49,13 @@ struct vpx_tlv {
char tlv[1];
};
+static inline int
+vpx_ws_err(const struct req *req)
+{
+ VSL(SLT_Error, req->sp->vxid, "insufficient workspace");
+ return (-1);
+}
+
/**********************************************************************
* PROXY 1 protocol
*/
@@ -128,10 +135,19 @@ vpx_proto1(const struct worker *wrk, const struct req *req)
freeaddrinfo(res);
return (-1);
}
- SES_Reserve_client_addr(req->sp, &sa);
+ if (! SES_Reserve_client_addr(req->sp, &sa)) {
+ freeaddrinfo(res);
+ return (vpx_ws_err(req));
+ }
AN(VSA_Build(sa, res->ai_addr, res->ai_addrlen));
- SES_Set_String_Attr(req->sp, SA_CLIENT_IP, fld[1]);
- SES_Set_String_Attr(req->sp, SA_CLIENT_PORT, fld[3]);
+ if (! SES_Set_String_Attr(req->sp, SA_CLIENT_IP, fld[1])) {
+ freeaddrinfo(res);
+ return (vpx_ws_err(req));
+ }
+ if (! SES_Set_String_Attr(req->sp, SA_CLIENT_PORT, fld[3])) {
+ freeaddrinfo(res);
+ return (vpx_ws_err(req));
+ }
freeaddrinfo(res);
i = getaddrinfo(fld[2], fld[4], &hints, &res);
@@ -149,7 +165,10 @@ vpx_proto1(const struct worker *wrk, const struct req *req)
freeaddrinfo(res);
return (-1);
}
- SES_Reserve_server_addr(req->sp, &sa);
+ if (! SES_Reserve_server_addr(req->sp, &sa)) {
+ freeaddrinfo(res);
+ return (vpx_ws_err(req));
+ }
AN(VSA_Build(sa, res->ai_addr, res->ai_addrlen));
freeaddrinfo(res);
@@ -403,14 +422,16 @@ vpx_proto2(const struct worker *wrk, struct req *req)
/* dst/server */
memcpy(&sin4.sin_addr, p + 20, 4);
memcpy(&sin4.sin_port, p + 26, 2);
- SES_Reserve_server_addr(req->sp, &sa);
+ if (! SES_Reserve_server_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
AN(VSA_Build(sa, &sin4, sizeof sin4));
VTCP_name(sa, ha, sizeof ha, pa, sizeof pa);
/* src/client */
memcpy(&sin4.sin_addr, p + 16, 4);
memcpy(&sin4.sin_port, p + 24, 2);
- SES_Reserve_client_addr(req->sp, &sa);
+ if (! SES_Reserve_client_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
AN(VSA_Build(sa, &sin4, sizeof sin4));
break;
case 0x21:
@@ -429,14 +450,16 @@ vpx_proto2(const struct worker *wrk, struct req *req)
/* dst/server */
memcpy(&sin6.sin6_addr, p + 32, 16);
memcpy(&sin6.sin6_port, p + 50, 2);
- SES_Reserve_server_addr(req->sp, &sa);
+ if (! SES_Reserve_server_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
AN(VSA_Build(sa, &sin6, sizeof sin6));
VTCP_name(sa, ha, sizeof ha, pa, sizeof pa);
/* src/client */
memcpy(&sin6.sin6_addr, p + 16, 16);
memcpy(&sin6.sin6_port, p + 48, 2);
- SES_Reserve_client_addr(req->sp, &sa);
+ if (! SES_Reserve_client_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
AN(VSA_Build(sa, &sin6, sizeof sin6));
break;
default:
@@ -448,8 +471,10 @@ vpx_proto2(const struct worker *wrk, struct req *req)
AN(sa);
VTCP_name(sa, hb, sizeof hb, pb, sizeof pb);
- SES_Set_String_Attr(req->sp, SA_CLIENT_IP, hb);
- SES_Set_String_Attr(req->sp, SA_CLIENT_PORT, pb);
+ if (! SES_Set_String_Attr(req->sp, SA_CLIENT_IP, hb))
+ return (vpx_ws_err(req));
+ if (! SES_Set_String_Attr(req->sp, SA_CLIENT_PORT, pb))
+ return (vpx_ws_err(req));
VSL(SLT_Proxy, req->sp->vxid, "2 %s %s %s %s", hb, pb, ha, pa);
@@ -476,15 +501,13 @@ vpx_proto2(const struct worker *wrk, struct req *req)
return (-1);
}
tlv = WS_Alloc(req->sp->ws, sizeof *tlv + tlv_len);
- if (tlv == NULL) {
- VSL(SLT_ProxyGarbage, req->sp->vxid,
- "PROXY2: TLV overflows WS");
- return (-1);
- }
+ if (tlv == NULL)
+ return (vpx_ws_err(req));
INIT_OBJ(tlv, VPX_TLV_MAGIC);
tlv->len = tlv_len;
memcpy(tlv->tlv, tlv_start, tlv_len);
- SES_Reserve_proxy_tlv(req->sp, &up);
+ if (! SES_Reserve_proxy_tlv(req->sp, &up))
+ return (vpx_ws_err(req));
*up = (uintptr_t)tlv;
return (0);
}
diff --git a/bin/varnishtest/tests/o00005.vtc b/bin/varnishtest/tests/o00005.vtc
index 3ae1aacfe..d84b92d4d 100644
--- a/bin/varnishtest/tests/o00005.vtc
+++ b/bin/varnishtest/tests/o00005.vtc
@@ -5,7 +5,8 @@ server s1 {
txresp
} -start
-varnish v1 -proto "PROXY" -vcl+backend {
+varnish v1 -arg "-p pool_sess=0,0,0" -proto "PROXY" -vcl+backend {
+ import vtc;
import proxy;
sub vcl_deliver {
@@ -20,6 +21,7 @@ varnish v1 -proto "PROXY" -vcl+backend {
set resp.http.key = proxy.cert_key();
set resp.http.sign = proxy.cert_sign();
set resp.http.cn = proxy.client_cert_cn();
+ set resp.http.ws_free = vtc.workspace_free(session);
}
} -start
@@ -243,4 +245,68 @@ client c1 {
expect_close
} -run
+delay 1
+
varnish v1 -expect ws_session_overflow == 1
+
+# error handling elsewhere in the proxy code
+# request is the same as initial
+
+varnish v1 -cliok "param.set workspace_session 450"
+varnish v1 -cliok "param.set pool_sess 10,100,1"
+
+delay 1
+
+# get rid of the surplus session mpl
+client c10 -proxy1 "1.2.3.4:1111 5.6.7.8:5678" {
+ txreq
+ rxresp
+} -start
+client c11 -proxy1 "1.2.3.4:1111 5.6.7.8:5678" {
+ txreq
+ rxresp
+} -start
+client c12 -proxy1 "1.2.3.4:1111 5.6.7.8:5678" {
+ txreq
+ rxresp
+} -start
+client c13 -proxy1 "1.2.3.4:1111 5.6.7.8:5678" {
+ txreq
+ rxresp
+} -start
+client c14 -proxy1 "1.2.3.4:1111 5.6.7.8:5678" {
+ txreq
+ rxresp
+} -start
+
+client c10 -wait
+client c11 -wait
+client c12 -wait
+client c13 -wait
+client c14 -wait
+
+client c2 {
+ # PROXY2 with CRC32C TLV
+ sendhex {
+ 0d 0a 0d 0a 00 0d 0a 51 55 49 54 0a
+ 21 11 00 65
+ d9 46 b5 21
+ 5f 8e a8 22
+ ed 96
+ 01 bb
+ 03 00 04 95 03 ee 75
+ 01 00 02 68 32
+ 02 00 0a 68 6f 63 64 65 74 2e 6e 65 74
+ 20 00 3d
+ 01 00 00 00 00
+ 21 00 07 54 4c 53 76 31 2e 33
+ 25 00 05 45 43 32 35 36
+ 24 00 0a 52 53 41 2d 53 48 41 32 35 36
+ 23 00 16 41 45 41 44 2d 41 45 53 31 32 38
+ 2d 47 43 4d 2d 53 48 41 32 35 36
+ }
+ txreq
+ expect_close
+} -run
+
+varnish v1 -expect ws_session_overflow == 2
More information about the varnish-commit
mailing list