cache_ban: Forward compatibility for the binary ban representation

Our ban expressions (like "obj.age > 20s") are represented in a binary format
(see top of cache_ban.h) which allows for forward compatibility, yet at the
respective places we currently just trigger an assertion failure if we hit an
unknown argument or operator code.

This commit brings forward compatibility such that, when bans are loaded from
persistent storage into older code which does not yet support newly introduced
binary codes, we no longer panic.

Ban evaluation:

For bans, evaluating an expression to "true" is always "correct" in that the
cache would not deliver banned content. It might cause objects to be removed
from cache, but that is at least not incorrect. So the fail safe action this
code takes is to always evaluate unknown ban expressions to true.

CLI ban.list:

For unsupported ban expressions, the unknown argument or operator codes are
formatted as "(0x%02x)" with the string "UNSUPPORTED" as the user-specified
argument. For example:

	1740567193.765849     0 -  (0x20) > UNSUPPORTED && obj.http.foo ~ 377.266

(note that here the operator > is supported and printed as such, and the ban
 contains one unsupported and one supported expression)

Logging:

For each unsupported argument or operator code, an Error VSL is output exactly
once to vxid 0.

Statistics:

Whenever unsupported argument or operator codes are encountered, the newly added
counters MAIN.bans_inval_arg1 and MAIN.bans_inval_oper are incremented,
respectively.

Fixes #4288

Author: nigoroll

bin/varnishd/cache/cache_ban.c

@@ -39,6 +39,7 @@
 #include "cache_ban.h"
 #include "cache_objhead.h"
 
+#include "vbm.h"
 #include "vcli_serve.h"
 #include "vend.h"
 #include "vmb.h"
@@ -66,6 +67,10 @@ struct ban_test {
 	const void		*arg2_spec;
 };
 
+// mark invalid arg1/oper so we report only once
+static struct vbitmap *inval_oper_logged;
+static struct vbitmap *inval_arg1_logged;
+
 static const char * const arg_name[BAN_ARGARRSZ + 1] = {
 #define PVAR(a, b, c) [BAN_ARGIDX(c)] = (a),
 #include "tbl/ban_vars.h"
@@ -215,6 +220,8 @@ ban_get_lump(const uint8_t **bs)
 
 /*--------------------------------------------------------------------
  * Pick a test apart from a spec string
+ *
+ * NOTICE: This code must be invariant to unknown arg1 and oper
  */
 
 static void
@@ -486,6 +493,30 @@ BAN_Time(const struct ban *b)
  * Evaluate ban-spec
  */
 
+static void
+ban_inval_arg1(uint8_t arg1)
+{
+	Lck_Lock(&ban_mtx);
+	VSC_C_main->bans_inval_arg1++;
+	Lck_Unlock(&ban_mtx);
+	if (vbit_test(inval_arg1_logged, arg1))
+		return;
+	vbit_set(inval_arg1_logged, arg1);
+	VSL(SLT_Error, NO_VXID, "Unsupported ban argument 0x%02x", arg1);
+}
+
+static void
+ban_inval_oper(uint8_t oper)
+{
+	Lck_Lock(&ban_mtx);
+	VSC_C_main->bans_inval_oper++;
+	Lck_Unlock(&ban_mtx);
+	if (vbit_test(inval_oper_logged, oper))
+		return;
+	vbit_set(inval_oper_logged, oper);
+	VSL(SLT_Error, NO_VXID, "Unsupported ban operator 0x%02x", oper);
+}
+
 int
 ban_evaluate(struct worker *wrk, const uint8_t *bsarg, struct objcore *oc,
     const struct http *reqhttp, unsigned *tests)
@@ -547,7 +578,8 @@ ban_evaluate(struct worker *wrk, const uint8_t *bsarg, struct objcore *oc,
 			darg2 = bt.arg2_double;
 			break;
 		default:
-			WRONG("Wrong BAN_ARG code");
+			ban_inval_arg1(bt.arg1);
+			continue;
 		}
 
 		switch (bt.oper) {
@@ -608,7 +640,7 @@ ban_evaluate(struct worker *wrk, const uint8_t *bsarg, struct objcore *oc,
 				return (0);
 			break;
 		default:
-			WRONG("Wrong BAN_OPER code");
+			ban_inval_oper(bt.oper);
 		}
 	}
 	return (1);
@@ -794,6 +826,27 @@ ccf_ban(struct cli *cli, const char * const *av, void *priv)
 		bprintf((buf), "%jus", dec);				\
 	} while (0)
 
+static void
+ban_render_inval(struct cli *cli, uint8_t arg1, uint8_t oper)
+{
+	if (! IS_BAN_ARG(arg1))
+		ban_inval_arg1(arg1);
+	if (! IS_BAN_OPER(oper))
+		ban_inval_oper(oper);
+
+	if (IS_BAN_ARG(arg1))
+		VCLI_Out(cli, "%s", arg_name[BAN_ARGIDX(arg1)]);
+	else
+		VCLI_Out(cli, "(0x%02x)", arg1);
+
+	if (IS_BAN_OPER(oper))
+		VCLI_Out(cli, " %s ", ban_oper[BAN_OPERIDX(oper)]);
+	else
+		VCLI_Out(cli, " (0x%02x) ", oper);
+
+	VCLI_Out(cli, "UNSUPPORTED");
+}
+
 static void
 ban_render(struct cli *cli, const uint8_t *bs, int quote)
 {
@@ -805,6 +858,13 @@ ban_render(struct cli *cli, const uint8_t *bs, int quote)
 	bs += BANS_HEAD_LEN;
 	while (bs < be) {
 		ban_iter(&bs, &bt);
+		if (UNLIKELY(! IS_BAN_ARG(bt.arg1) || ! IS_BAN_OPER(bt.oper))) {
+			ban_render_inval(cli, bt.arg1, bt.oper);
+			if (bs < be)
+				VCLI_Out(cli, " && ");
+			continue;
+		}
+
 		ASSERT_BAN_ARG(bt.arg1);
 		ASSERT_BAN_OPER(bt.oper);
 
@@ -985,6 +1045,12 @@ BAN_Init(void)
 {
 	struct ban_proto *bp;
 
+	// uses malloc where static could be used, but only once
+	inval_oper_logged = vbit_new(256);
+	inval_arg1_logged = vbit_new(256);
+	AN(inval_oper_logged);
+	AN(inval_arg1_logged);
+
 	BAN_Build_Init();
 	Lck_New(&ban_mtx, lck_ban);
 	CLI_AddFuncs(ban_cmds);
@@ -1028,4 +1094,9 @@ BAN_Shutdown(void)
 	Lck_Unlock(&ban_mtx);
 
 	BAN_Build_Fini();
+
+	vbit_destroy(inval_oper_logged);
+	vbit_destroy(inval_arg1_logged);
+	inval_oper_logged = NULL;
+	inval_arg1_logged = NULL;
 }

bin/varnishd/cache/cache_ban.h

@@ -86,7 +86,8 @@
 
 #define BAN_OPERIDX(x) ((x) - BANS_OPER_OFF_)
 #define BAN_OPERARRSZ  (BANS_OPER_LIM_ - BANS_OPER_OFF_)
-#define ASSERT_BAN_OPER(x) assert((x) >= BANS_OPER_OFF_ && (x) < BANS_OPER_LIM_)
+#define IS_BAN_OPER(x) ((x) >= BANS_OPER_OFF_ && (x) < BANS_OPER_LIM_)
+#define ASSERT_BAN_OPER(x) assert(IS_BAN_OPER(x))
 
 #define BANS_ARG_URL		0x18
 #define BANS_ARG_OFF_		BANS_ARG_URL
@@ -101,7 +102,8 @@
 
 #define BAN_ARGIDX(x) ((x) - BANS_ARG_OFF_)
 #define BAN_ARGARRSZ  (BANS_ARG_LIM - BANS_ARG_OFF_)
-#define ASSERT_BAN_ARG(x) assert((x) >= BANS_ARG_OFF_ && (x) < BANS_ARG_LIM)
+#define IS_BAN_ARG(x) ((x) >= BANS_ARG_OFF_ && (x) < BANS_ARG_LIM)
+#define ASSERT_BAN_ARG(x) assert(IS_BAN_ARG(x))
 
 // has an arg1_spec (BANS_FLAG_HTTP at build time)
 #define BANS_HAS_ARG1_SPEC(arg)	\

lib/libvsc/VSC_main.vsc

@@ -925,6 +925,22 @@
 	Number of extra bytes accumulated through dropped and completed
 	bans in the persistent ban lists.
 
+.. varnish_vsc:: bans_inval_arg1
+	:group: ban_mtx
+	:oneliner:	Unsupported ban argument encountered
+
+	Number of times an unsupported ban argument has been encountered.
+
+	Unsupported ban arguments always evaluate true.
+
+.. varnish_vsc:: bans_inval_oper
+	:group: ban_mtx
+	:oneliner:	Unsupported ban operator encountered
+
+	Number of times an unsupported ban operator has been encountered.
+
+	Unsupported ban operators always evaluate true.
+
 .. varnish_vsc:: n_purges
 	:oneliner:	Number of purge operations executed