[6.0] efaa633ca Add Session Attribute workspace overflow handling
Martin Blix Grydeland
martin at varnish-software.com
Tue Feb 4 10:02:08 UTC 2020
commit efaa633ca71ad09bd7f68c5d7fb1886b15a92d1e
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 f5f102075..0029a68d3 100644
--- a/bin/varnishd/cache/cache_acceptor.c
+++ b/bin/varnishd/cache/cache_acceptor.c
@@ -319,17 +319,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);
@@ -342,13 +342,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 8c9325515..debb0c2cf 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -104,8 +104,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;
@@ -114,12 +114,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) \
@@ -135,16 +137,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;
@@ -158,8 +160,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 5d036f6da..939a0fe17 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -351,9 +351,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 1c216dc78..91b8e2695 100644
--- a/bin/varnishd/http1/cache_http1_fsm.c
+++ b/bin/varnishd/http1/cache_http1_fsm.c
@@ -113,7 +113,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 140007ffc..ab71cc2aa 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/bin/varnishd/http2/cache_http2_session.c
@@ -102,7 +102,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) {
@@ -131,7 +131,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 9232254d3..0c7be9bb3 100644
--- a/bin/varnishd/proxy/cache_proxy_proto.c
+++ b/bin/varnishd/proxy/cache_proxy_proto.c
@@ -50,6 +50,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
*/
@@ -109,17 +116,23 @@ vpx_proto1(const struct worker *wrk, const struct req *req)
return (-1);
}
- SES_Reserve_client_addr(req->sp, &sa);
+ if (! SES_Reserve_client_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
+
if (VSS_ResolveOne(sa, fld[1], fld[3],
pfam, SOCK_STREAM, AI_NUMERICHOST | AI_NUMERICSERV) == NULL) {
VSL(SLT_ProxyGarbage, req->sp->vxid,
"PROXY1: Cannot resolve source address");
return (-1);
}
- 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]))
+ return (vpx_ws_err(req));
+ if (! SES_Set_String_Attr(req->sp, SA_CLIENT_PORT, fld[3]))
+ return (vpx_ws_err(req));
+
+ if (! SES_Reserve_server_addr(req->sp, &sa))
+ return (vpx_ws_err(req));
- SES_Reserve_server_addr(req->sp, &sa);
if (VSS_ResolveOne(sa, fld[2], fld[4],
pfam, SOCK_STREAM, AI_NUMERICHOST | AI_NUMERICSERV) == NULL) {
VSL(SLT_ProxyGarbage, req->sp->vxid,
@@ -379,14 +392,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:
@@ -405,14 +420,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:
@@ -424,8 +441,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);
@@ -452,15 +471,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