From af32ae14bc68a374cf596a78b64c7fba5aa04366 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Wed, 26 Feb 2025 12:58:03 +0100 Subject: [PATCH] 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 --- bin/varnishd/cache/cache_ban.c | 75 +++++++++++++++++++++++++++++++++- bin/varnishd/cache/cache_ban.h | 6 ++- lib/libvsc/VSC_main.vsc | 16 ++++++++ 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/bin/varnishd/cache/cache_ban.c b/bin/varnishd/cache/cache_ban.c index d611445d60a..bbc5715f48b 100644 --- a/bin/varnishd/cache/cache_ban.c +++ b/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; } diff --git a/bin/varnishd/cache/cache_ban.h b/bin/varnishd/cache/cache_ban.h index d3339eef02d..48c86131333 100644 --- a/bin/varnishd/cache/cache_ban.h +++ b/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) \ diff --git a/lib/libvsc/VSC_main.vsc b/lib/libvsc/VSC_main.vsc index 22097f2d8a0..739d641b8a2 100644 --- a/lib/libvsc/VSC_main.vsc +++ b/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