From f74dff4df191af1707139c798ee1ebe59422ed49 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Fri, 10 Jul 2026 11:24:49 +0200 Subject: [PATCH 1/2] Made format function collect data container Ticket: CFE-3105 Changelog: Title Signed-off-by: Victor Moene --- libpromises/evalfunction.c | 89 ++++++++++--------- .../acceptance/01_vars/02_functions/format.cf | 2 + 2 files changed, 51 insertions(+), 40 deletions(-) diff --git a/libpromises/evalfunction.c b/libpromises/evalfunction.c index 922149a359..f6ee93b104 100644 --- a/libpromises/evalfunction.c +++ b/libpromises/evalfunction.c @@ -6293,7 +6293,7 @@ static FnCallResult FnCallFormat(EvalContext *ctx, ARG_UNUSED const Policy *poli { const char *format_piece = BufferData(SeqAt(s, 1)); bool percent = StringEqualN(format_piece, "%%", 2); - char *data = NULL; + const Rlist *arg = NULL; if (percent) { @@ -6301,10 +6301,10 @@ static FnCallResult FnCallFormat(EvalContext *ctx, ARG_UNUSED const Policy *poli } else if (rp != NULL) { - data = RlistScalarValue(rp); + arg = rp; rp = rp->next; } - else // not %% and no data + else // not %% and no arg { Log(LOG_LEVEL_ERR, "format() didn't have enough parameters"); BufferDestroy(buf); @@ -6333,7 +6333,7 @@ static FnCallResult FnCallFormat(EvalContext *ctx, ARG_UNUSED const Policy *poli if (strrchr(format_piece, 'd') != NULL || strrchr(format_piece, 'o') != NULL || strrchr(format_piece, 'x') != NULL) { long x = 0; - sscanf(data, "%ld", &x); + sscanf(RlistScalarValue(arg), "%ld", &x); snprintf(piece, CF_BUFSIZE, format_piece, x); BufferAppend(buf, piece, strlen(piece)); } @@ -6345,13 +6345,13 @@ static FnCallResult FnCallFormat(EvalContext *ctx, ARG_UNUSED const Policy *poli else if (strrchr(format_piece, 'f') != NULL) { double x = 0; - sscanf(data, "%lf", &x); + sscanf(RlistScalarValue(arg), "%lf", &x); snprintf(piece, CF_BUFSIZE, format_piece, x); BufferAppend(buf, piece, strlen(piece)); } else if (strrchr(format_piece, 's') != NULL) { - BufferAppendF(buf, format_piece, data); + BufferAppendF(buf, format_piece, RlistScalarValue(arg)); } else if (strrchr(format_piece, 'S') != NULL) { @@ -6370,50 +6370,59 @@ static FnCallResult FnCallFormat(EvalContext *ctx, ARG_UNUSED const Policy *poli ProgrammingError("Couldn't find the expected S format spec in %s", format_piece); } - const char* const varname = data; - VarRef *ref = VarRefParse(varname); - DataType type; - const void *value = EvalContextVariableGet(ctx, ref, &type); - VarRefDestroy(ref); - - if (type == CF_DATA_TYPE_CONTAINER) + if (arg->val.type == RVAL_TYPE_CONTAINER) { Writer *w = StringWriter(); - JsonWriteCompact(w, value); + JsonWriteCompact(w, (JsonElement*) arg->val.item); BufferAppendF(buf, format_rewrite, StringWriterData(w)); WriterClose(w); } - else // it might be a list reference - { - DataType data_type; - const Rlist *list = GetListReferenceArgument(ctx, fp, varname, &data_type); - if (data_type == CF_DATA_TYPE_STRING_LIST) + else { + const char* const varname = RlistScalarValue(arg); + VarRef *ref = VarRefParse(varname); + DataType type; + const void *value = EvalContextVariableGet(ctx, ref, &type); + VarRefDestroy(ref); + + if (type == CF_DATA_TYPE_CONTAINER) { Writer *w = StringWriter(); - WriterWrite(w, "{ "); - for (const Rlist *rp = list; rp; rp = rp->next) - { - char *escaped = EscapeCharCopy(RlistScalarValue(rp), '"', '\\'); - WriterWriteF(w, "\"%s\"", escaped); - free(escaped); - - if (rp != NULL && rp->next != NULL) - { - WriterWrite(w, ", "); - } - } - WriterWrite(w, " }"); - + JsonWriteCompact(w, value); BufferAppendF(buf, format_rewrite, StringWriterData(w)); WriterClose(w); } - else // whatever this is, it's not a list reference or a data container + else // it might be a list reference { - Log(LOG_LEVEL_VERBOSE, "format() with %%S specifier needs a data container or a list instead of '%s'.", - varname); - BufferDestroy(buf); - SeqDestroy(s); - return FnFailure(); + DataType data_type; + const Rlist *list = GetListReferenceArgument(ctx, fp, varname, &data_type); + if (data_type == CF_DATA_TYPE_STRING_LIST) + { + Writer *w = StringWriter(); + WriterWrite(w, "{ "); + for (const Rlist *tmp = list; tmp; tmp = tmp->next) + { + char *escaped = EscapeCharCopy(RlistScalarValue(tmp), '"', '\\'); + WriterWriteF(w, "\"%s\"", escaped); + free(escaped); + + if (tmp != NULL && tmp->next != NULL) + { + WriterWrite(w, ", "); + } + } + WriterWrite(w, " }"); + + BufferAppendF(buf, format_rewrite, StringWriterData(w)); + WriterClose(w); + } + else // whatever this is, it's not a list reference or a data container + { + Log(LOG_LEVEL_VERBOSE, "format() with %%S specifier needs a data container or a list instead of '%s'.", + varname); + BufferDestroy(buf); + SeqDestroy(s); + return FnFailure(); + } } } } @@ -11612,7 +11621,7 @@ const FnCallType CF_FNCALL_TYPES[] = FnCallTypeNew("findprocesses", CF_DATA_TYPE_CONTAINER, PROCESSEXISTS_ARGS, &FnCallProcessExists, "Returns data container of processes matching the regular expression", FNCALL_OPTION_CACHED, FNCALL_CATEGORY_SYSTEM, SYNTAX_STATUS_NORMAL, DEFAULT_ARGC), FnCallTypeNew("format", CF_DATA_TYPE_STRING, FORMAT_ARGS, &FnCallFormat, "Applies a list of string values in arg2,arg3... to a string format in arg1 with sprintf() rules", - FNCALL_OPTION_VARARG, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(1, -1)), + FNCALL_OPTION_VARARG | FNCALL_OPTION_COLLECTING, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(1, -1)), FnCallTypeNew("getclassmetatags", CF_DATA_TYPE_STRING_LIST, GETCLASSMETATAGS_ARGS, &FnCallGetMetaTags, "Collect the class arg1's meta tags into an slist, optionally collecting only tag key arg2", FNCALL_OPTION_VARARG, FNCALL_CATEGORY_UTILS, SYNTAX_STATUS_NORMAL, ARGC(1, -1)), FnCallTypeNew("getenv", CF_DATA_TYPE_STRING, GETENV_ARGS, &FnCallGetEnv, "Return the environment variable named arg1, truncated at arg2 characters", diff --git a/tests/acceptance/01_vars/02_functions/format.cf b/tests/acceptance/01_vars/02_functions/format.cf index a9cf432a9b..f2e3c0488d 100644 --- a/tests/acceptance/01_vars/02_functions/format.cf +++ b/tests/acceptance/01_vars/02_functions/format.cf @@ -46,6 +46,7 @@ bundle edit_line init_insert "key='S_123_list' value='{ \"one\", \"two\", \"\\\"three\\\"\" }'"; "key='S_container_1' value='[null]'"; "key='S_container_2' value='[{\"x\":123},\"yz\"]'"; + "key='S_function' value='{\"hello\": \"world\"}'"; } ####################################################### @@ -82,6 +83,7 @@ bundle agent test "array[S_container_1]" string => format("%S", mycontainer1); "array[S_container_2]" string => format("%S", mycontainer2); "formatted" slist => maparray("key='$(this.k)' value='$(this.v)'", "array"); + "array[S_function]" string => format("%S", parsejson('{"hello": "world"}')); files: "$(G.testfile).actual" From 10d94b82aec6fd253087acc4f53972c564a9c4a4 Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Fri, 10 Jul 2026 11:29:20 +0200 Subject: [PATCH 2/2] remove codql just to run the whole gh actions --- .github/codeql/codeql-config.yml | 5 -- .../cpp-queries/bool-type-mismatch-return.c | 62 --------------- .../bool-type-mismatch-return.qhelp | 66 ---------------- .../cpp-queries/bool-type-mismatch-return.ql | 76 ------------------- .../cpp-queries/missing-argument-null-check.c | 39 ---------- .../missing-argument-null-check.qhelp | 49 ------------ .../missing-argument-null-check.ql | 37 --------- .github/codeql/cpp-queries/qlpack.yml | 4 - .github/workflows/codeql.yml | 53 ------------- 9 files changed, 391 deletions(-) delete mode 100644 .github/codeql/codeql-config.yml delete mode 100644 .github/codeql/cpp-queries/bool-type-mismatch-return.c delete mode 100644 .github/codeql/cpp-queries/bool-type-mismatch-return.qhelp delete mode 100644 .github/codeql/cpp-queries/bool-type-mismatch-return.ql delete mode 100644 .github/codeql/cpp-queries/missing-argument-null-check.c delete mode 100644 .github/codeql/cpp-queries/missing-argument-null-check.qhelp delete mode 100644 .github/codeql/cpp-queries/missing-argument-null-check.ql delete mode 100644 .github/codeql/cpp-queries/qlpack.yml delete mode 100644 .github/workflows/codeql.yml diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml deleted file mode 100644 index b082519997..0000000000 --- a/.github/codeql/codeql-config.yml +++ /dev/null @@ -1,5 +0,0 @@ -name: "CFEngine cpp CodeQL config" - -queries: - - uses: cfengine/core/.github/codeql/cpp-queries/bool-type-mismatch-return.ql@master - - uses: cfengine/core/.github/codeql/cpp-queries/missing-argument-null-check.ql@master diff --git a/.github/codeql/cpp-queries/bool-type-mismatch-return.c b/.github/codeql/cpp-queries/bool-type-mismatch-return.c deleted file mode 100644 index bb506d157c..0000000000 --- a/.github/codeql/cpp-queries/bool-type-mismatch-return.c +++ /dev/null @@ -1,62 +0,0 @@ -#include - -int good_int() -{ - return 0; -} - -int bad_int() -{ - return false; -} - -bool good_with_variable() -{ - bool r = true; - return r; -} - -bool bad_with_variable() -{ - int r = true; - return r; -} - -bool good_with_constant() -{ - return false; -} - -bool bad_with_constant() -{ - return 0; -} - -bool good_with_function() -{ - return good_with_constant(); -} - -bool bad_with_function() -{ - return good_int(); -} - -bool good_with_comparison() -{ - return good_int() != 0; -} - -int main(void) -{ - good_int(); - bad_int(); - good_with_variable(); - bad_with_variable(); - good_with_constant(); - bad_with_constant(); - good_with_function(); - bad_with_function(); - good_with_comparison(); - return 0; -} diff --git a/.github/codeql/cpp-queries/bool-type-mismatch-return.qhelp b/.github/codeql/cpp-queries/bool-type-mismatch-return.qhelp deleted file mode 100644 index c9c53ee4f0..0000000000 --- a/.github/codeql/cpp-queries/bool-type-mismatch-return.qhelp +++ /dev/null @@ -1,66 +0,0 @@ - - - - - -

-Boolean type mismatch between function type and return value. -This can be the result of copy-pasted code, which should have been modified. -

- -

-In C, most things we think of as boolean are actually int (0 or 1). -We prefer to use bool return type for functions which have 2 return values (true or false). -We consider some different things boolean: -

- -
    -
  • -A variable with type bool -
    • -
  • -A function call with return type bool -
    • -
  • -A boolean macro - true or false -
    • -
  • -A comparison - == or != (or less than / greater than variants). -
    • -
  • -A logical operation - AND or OR. -
    • -
- -

-The reason why this query was added was to catch cases like a boolean function returning -1. -int functions typically return 0 for success and -1 for failure. -This means that if you copied the error handling of an int function to a bool function, it would return true (success) in case of error. -Error handling is the main reason to have this strict type checking for bool. -

- - - - -

-Change the returned value to something boolean, or change the function return type to bool. -Sometimes this means adding an an explicit comparison in the return statement. -(Typecasting is almost never the right answer). -

- - - - - - - - - -
  • -CFEngine Contribution guidelines: CONTRIBUTING.md -
    • - -     -     diff --git a/.github/codeql/cpp-queries/bool-type-mismatch-return.ql b/.github/codeql/cpp-queries/bool-type-mismatch-return.ql deleted file mode 100644 index e8ce038bfd..0000000000 --- a/.github/codeql/cpp-queries/bool-type-mismatch-return.ql +++ /dev/null @@ -1,76 +0,0 @@ -/** - * @name Boolean type mismatch between function type and return value - * @description Strict boolean type-checking for return statements. - * Boolean functions must return something boolean. - * Non-boolean functions must not return something boolean. - * Comparisons, logical AND/OR, and true/false macros are bool. - * @kind problem - * @problem.severity warning - * @id cpp/bool-type-mismatch-return - * @tags readability - * correctness - * @precision very-high - */ - -import cpp - -predicate isBoolExpr(Expr expression){ - // Not bool if there is an explicit cast to something else: - not exists(CStyleCast cast | - cast.getType().getName() != "bool" - and - cast.getExplicitlyConverted() = expression.getExplicitlyConverted() - ) - and - // One of these imply boolean: - ( - // variable of type bool: - expression.getType().getName() = "bool" - // true or false macro: - or exists(MacroInvocation m | - m.getExpr() = expression - and (m.getMacroName() = "true" or m.getMacroName() = "false")) - // && or || operator - or exists(BinaryLogicalOperation b | - b = expression) - // == or != or > or < or >= or <= operator: - or exists(ComparisonOperation cmp | - cmp = expression) - // ! operator: - or exists(NotExpr n | - n = expression - and isBoolExpr(n.getOperand())) - // Recursively check both branches of ternary operator: - or exists(ConditionalExpr c | - c = expression - and isBoolExpr(c.getThen()) - and isBoolExpr(c.getElse())) - ) -} - -string showMacroExpr(Expr e){ - not e.isInMacroExpansion() - and result = e.toString() -or - e.isInMacroExpansion() // Show true=1/false=0 in alert - and result = e.findRootCause() - .toString() - .replaceAll("#define ", "") - .replaceAll(" ", "=") -} - -from Function f, ReturnStmt r, string rt -where r.getEnclosingFunction() = f - and ( - f.getType().getName() = "bool" - and not isBoolExpr(r.getExpr()) - and rt = "non-bool" - or - f.getType().getName() != "bool" - and isBoolExpr(r.getExpr()) - and rt = "bool" - ) -select r, "Function " + f.getName() + - " has return type " + f.getType().getName() + - " and returns " + rt + - "(" + showMacroExpr(r.getExpr()) + ")" diff --git a/.github/codeql/cpp-queries/missing-argument-null-check.c b/.github/codeql/cpp-queries/missing-argument-null-check.c deleted file mode 100644 index 7a95060e0f..0000000000 --- a/.github/codeql/cpp-queries/missing-argument-null-check.c +++ /dev/null @@ -1,39 +0,0 @@ -#include - -typedef struct { - char *string; -} StructWithString; - -void good_with_assert(StructWithString *data) -{ - assert(data != NULL); - char *string = data->string; -} - -void good_with_no_deref(StructWithString *data) -{ - good_with_assert(data); -} - -void good_with_if(StructWithString *data) -{ - if (data != NULL) - { - char *string = data->string; - } -} - -void bad_deref(StructWithString *data) -{ - char *string = data->string; -} - -int main(void) -{ - StructWithString *data = NULL; - good_with_no_deref(data); // Doesn't dereference, so no problem - good_with_assert(data); // Assert will detect our error - good_with_if(data); // Works with NULL pointers - bad_deref(data); // Blows up - will be detected in alert - return 0; -} diff --git a/.github/codeql/cpp-queries/missing-argument-null-check.qhelp b/.github/codeql/cpp-queries/missing-argument-null-check.qhelp deleted file mode 100644 index c283a4220d..0000000000 --- a/.github/codeql/cpp-queries/missing-argument-null-check.qhelp +++ /dev/null @@ -1,49 +0,0 @@ - - - - - -

    -Functions which dereference a pointer should test for NULL. -This should be done as an explicit comparison in an assert or if statement. -If the function assumes that an argument is non-null, add an assert at the beginning of the function body. -

    - -

    -There are some limitations in the current implementation. -It only looks for explicit comparisons, if (ptr == NULL), but this is the correct way to write it, according to our style guidelines. -Only arrow syntax is detected, ptr->field, so it should be expanded to also detect asterisk syntax, *ptr. -Also, it doesn't check that the comparison is before the dereference, it should be improved to also alert in cases where the check is after the dereference. -There shouldn't be false positives, but it should be expanded to find more problematic cases. -

    - -     - - -

    -Add an assert to the beginning of the function if it assumes the argument is not null: assert(ptr != NULL). -Add an an explicit comparison somewhere in the function if it is okay for the argument to be NULL. -(Usually this should be an if around the dereference). -Note that in both cases, the comparison must be explicit (using == NULL or != NULL). -

    - -     - - -

    -This example has 2 correct (good) functions, and one incorrect (bad) function: -

    - - - -     - - -
  • -CFEngine Contribution guidelines: CONTRIBUTING.md -
    • - -     -     diff --git a/.github/codeql/cpp-queries/missing-argument-null-check.ql b/.github/codeql/cpp-queries/missing-argument-null-check.ql deleted file mode 100644 index 3ecddfcce4..0000000000 --- a/.github/codeql/cpp-queries/missing-argument-null-check.ql +++ /dev/null @@ -1,37 +0,0 @@ -/** - * @name Pointer argument is dereferenced without checking for NULL - * @description Functions which dereference a pointer should test for NULL. - * This should be done as an explicit comparison in an assert or if statement. - * @kind problem - * @problem.severity recommendation - * @id cpp/missing-argument-null-check - * @tags readability - * correctness - * safety - * @precision very-high - */ - -import cpp - -predicate hasNullCheck(Function func, Parameter p){ - exists(MacroInvocation m | - m.getMacroName() = "assert" - and m.getEnclosingFunction() = func - and m.getUnexpandedArgument(0) = p.getName() + " != NULL") - or - exists(EqualityOperation comparison, MacroInvocation m| - comparison.getEnclosingFunction() = func - and comparison.getLeftOperand().toString() = p.getName() - and comparison.getRightOperand() = m.getExpr() - and m.getMacroName() = "NULL") -} - -from Function func, PointerFieldAccess acc, Parameter p, PointerType pt -where acc.getEnclosingFunction() = func - and p.getFunction() = func - and p.getType() = pt - and acc.getQualifier().toString() = p.getName() - and not hasNullCheck(func, p) -select acc, "Parameter " + p.getName() + - " in " + func.getName() + - "() is dereferenced without an explicit null-check" diff --git a/.github/codeql/cpp-queries/qlpack.yml b/.github/codeql/cpp-queries/qlpack.yml deleted file mode 100644 index 3be7e4e80c..0000000000 --- a/.github/codeql/cpp-queries/qlpack.yml +++ /dev/null @@ -1,4 +0,0 @@ -name: cfengine/codeql-cpp-queries -version: 1.0.0 -dependencies: - codeql/cpp-all: ~0.5.4 diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml deleted file mode 100644 index 996b8b8707..0000000000 --- a/.github/workflows/codeql.yml +++ /dev/null @@ -1,53 +0,0 @@ -name: "CodeQL" - -on: - push: - branches: [ "master", "3.27.x", "3.24.x" ] - pull_request: - branches: [ "master", "3.27.x", "3.24.x" ] - schedule: - - cron: "40 18 * * 6" - -jobs: - analyze: - name: Analyze - runs-on: ubuntu-latest - permissions: - actions: read - contents: read - security-events: write - - strategy: - fail-fast: false - matrix: - language: [ python, cpp ] - - steps: - - name: Checkout - uses: actions/checkout@v3 - with: - submodules: recursive - - - name: Initialize CodeQL - uses: github/codeql-action/init@v3 - with: - languages: ${{ matrix.language }} - queries: +security-and-quality - config-file: .github/codeql/codeql-config.yml - - - name: Autobuild (Python) - if: ${{ matrix.language == 'python' }} - uses: github/codeql-action/autobuild@v3 - - - name: Install dependencies (C) - if: ${{ matrix.language == 'cpp' }} - run: ./ci/dependencies.sh - - - name: Build (C) - if: ${{ matrix.language == 'cpp' }} - run: ./autogen.sh && make - - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v3 - with: - category: "/language:${{ matrix.language }}"