[master] 287dc4a67 Add Session Attribute workspace overflow handling
Nils Goroll
nils.goroll at uplex.de
Tue Nov 26 16:09:06 UTC 2019
commit 287dc4a6745c374e0b229bfa861d664989a3a9e8
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 #3145
diff --git a/bin/varnishd/cache/cache_acceptor.c b/bin/varnishd/cache/cache_acceptor.c
index 73430d76e..0e7ee9877 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 d536962bf..3e131943b 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -110,8 +110,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;
@@ -120,12 +120,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) \
@@ -143,16 +145,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); \
+ 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;
@@ -164,8 +166,10 @@ SES_Set_String_Attr(struct sess *sp, enum sess_attr a, const char *src)
if (strcmp(sess_attr[a].type, "char"))
WRONG("wrong sess_attr: not char");
- 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 b8ccb4704..b9e7e32ca 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -389,9 +389,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);
/* cache_shmlog.c */
extern struct VSC_main *VSC_C_main;
diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
index 9c0998550..b5536d545 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 aa91bc15d..0fce32bd8 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 78b120c2a..80d93349e 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,
@@ -377,14 +390,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:
@@ -403,14 +418,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:
@@ -422,8 +439,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);
@@ -450,15 +469,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