From 29e60a9ba05016847c3b601f469bcb5e19147960 Mon Sep 17 00:00:00 2001 From: Alexis Mousset Date: Thu, 7 Sep 2023 15:25:18 +0200 Subject: [PATCH 1/2] Added the function name to the result cache key The function cache only used the args values, which in some cases could lead to mixing results from different functions with the same arguments. Ticket: CFE-4244 Changelog: Cashed policy function results now take into account number of arguments and function name. Signed-off-by: Lars Erik Wik Co-authored-by: Alexis Mousset --- libpromises/eval_context.c | 18 ++++++- .../01_vars/02_functions/cache_name.cf | 51 +++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 tests/acceptance/01_vars/02_functions/cache_name.cf diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 7f56480f97..0422ea7649 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -2837,7 +2837,14 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, return false; } - Rval *rval = FuncCacheMapGet(ctx->function_cache, args); + // The cache key is made of the function name and all args values + Rlist *args_copy = RlistCopy(args); + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); + Rval *rval = FuncCacheMapGet(ctx->function_cache, key); + RlistDestroy(key); if (rval) { if (rval_out) @@ -2863,7 +2870,14 @@ void EvalContextFunctionCachePut(EvalContext *ctx, Rval *rval_copy = xmalloc(sizeof(Rval)); *rval_copy = RvalCopy(*rval); - FuncCacheMapInsert(ctx->function_cache, RlistCopy(args), rval_copy); + + Rlist *args_copy = RlistCopy(args); + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); + + FuncCacheMapInsert(ctx->function_cache, key, rval_copy); } /* cfPS and associated machinery */ diff --git a/tests/acceptance/01_vars/02_functions/cache_name.cf b/tests/acceptance/01_vars/02_functions/cache_name.cf new file mode 100644 index 0000000000..abbace9238 --- /dev/null +++ b/tests/acceptance/01_vars/02_functions/cache_name.cf @@ -0,0 +1,51 @@ +####################################################### +# +# Test that the function result cache checks function name +# +####################################################### + +body common control +{ + inputs => { "../../default.cf.sub" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### + + +bundle agent init +{ + vars: + "agent_regex" string => ".*cf-agent.*"; +} + +####################################################### + +bundle common test +{ + meta: + "description" -> { "CFE-4244" } + string => "Test that the function result cache checks function name"; + + vars: + "res1" data => findprocesses("${init.agent_regex}"); + + classes: + # must not reuse result from previous line + # is reused, produces a type error + "_pass" expression => processexists("${init.agent_regex}"); +} + + +####################################################### + +bundle agent check +{ + methods: + _pass:: + "pass" usebundle => dcs_pass("$(this.promise_filename)"); + + !_pass:: + "pass" usebundle => dcs_fail("$(this.promise_filename)"); +} From eec977631e9875aa5475aa24d5b276f77ef789a7 Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Thu, 14 Sep 2023 14:56:40 +0200 Subject: [PATCH 2/2] Moved asserts to top of EvalContextFunctionCache functions Two reasons for this: 1. CONTRIBUTING.md says to do this at the top 2. the `ctx` argument was actually used before the assert Ticket: None Changelog: None Signed-off-by: Lars Erik Wik --- libpromises/eval_context.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 0422ea7649..1d50f4eeab 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -2832,6 +2832,10 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, const FnCall *fp ARG_UNUSED, const Rlist *args, Rval *rval_out) { + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + if (!(ctx->eval_options & EVAL_OPTION_CACHE_SYSTEM_FUNCTIONS)) { return false; @@ -2839,9 +2843,6 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, // The cache key is made of the function name and all args values Rlist *args_copy = RlistCopy(args); - assert(fp != NULL); - assert(fp->name != NULL); - assert(ctx != NULL); Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); Rval *rval = FuncCacheMapGet(ctx->function_cache, key); RlistDestroy(key); @@ -2863,6 +2864,10 @@ void EvalContextFunctionCachePut(EvalContext *ctx, const FnCall *fp ARG_UNUSED, const Rlist *args, const Rval *rval) { + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + if (!(ctx->eval_options & EVAL_OPTION_CACHE_SYSTEM_FUNCTIONS)) { return; @@ -2872,9 +2877,6 @@ void EvalContextFunctionCachePut(EvalContext *ctx, *rval_copy = RvalCopy(*rval); Rlist *args_copy = RlistCopy(args); - assert(fp != NULL); - assert(fp->name != NULL); - assert(ctx != NULL); Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); FuncCacheMapInsert(ctx->function_cache, key, rval_copy);