Merge pull request #5414 from larsewi/no-action

ENT-11137: Warn if no action requested when parsing

cf-agent/verify_files.c

@@ -617,8 +617,9 @@ static PromiseResult VerifyFilePromise(EvalContext *ctx, char *path, const Promi
     free(chrooted_path);
     if (AttrHasNoAction(&a))
     {
-        Log(LOG_LEVEL_WARNING,
-            "No action was requested for file '%s'. Maybe a typo in the policy?", path);
+        Log(LOG_LEVEL_VERBOSE, "No action was requested for file '%s'. "
+            "Maybe all attributes are skipped due to unresolved arguments in policy functions? "
+            "Maybe a typo in the policy?", path);
     }
 
     switch(result)

libpromises/cf3parse.y

@@ -297,6 +297,9 @@ classpromises:         classpromise
 
 classpromise:          class
                      | promise_decl
+                       {
+                          ParserCheckPromiseLine();
+                       }
 
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
 

libpromises/cf3parse_logic.h

@@ -1058,4 +1058,66 @@ static inline void ParserHandleQuotedListItem()
     FREE_AND_NULL(P.currentstring);
 }
 
+/**
+ * A sanity check that prints a warning if there is a promise without any
+ * actions (i.e., the promise is a no-op). This check is naive and assumes
+ * promises containing any non-common attributes perform actions. It also has
+ * exceptions for promises that perform actions without any attributes. We can
+ * expect false negatives. However, there should not be any false positives. The
+ * motivation for this check is to aid policy writers in detecting semantic
+ * errors early.
+ */
+static inline void ParserCheckPromiseLine()
+{
+    if (P.currentpromise == NULL)
+    {
+        return;
+    }
+
+    const char *const promise_type = P.currenttype;
+    if (!IsBuiltInPromiseType(promise_type))
+    {
+        // We leave sanity checking to the custom promise module.
+        return;
+    }
+
+    // The following promise types does not require any actions
+    static const char *const exceptions[] = {
+        "classes", "commands", "methods", "reports", "insert_lines",
+        "delete_lines", "build_xpath", "insert_tree" };
+    static const size_t num_exceptions = sizeof(exceptions) / sizeof(exceptions[0]);
+
+    if (IsStringInArray(promise_type, exceptions, num_exceptions))
+    {
+        // This promise type does not require any action attributes.
+        return;
+    }
+
+    // We don't consider common attributes an actions.
+    static const char *const common_attrs[] = {
+        "action", "classes", "comment", "depends_on",
+        "handle", "if", "meta", "with" };
+
+    const Seq *const constraints = P.currentpromise->conlist;
+    const size_t num_constraints = SeqLength(constraints);
+    for (size_t i = 0; i < num_constraints; i++)
+    {
+        const Constraint *const constraint = SeqAt(constraints, i);
+        if (!IsStringInArray(constraint->lval, common_attrs,
+                             sizeof(common_attrs) / sizeof(common_attrs[0])))
+        {
+            // Not in common attributes, we assume it is an action
+            return;
+        }
+    }
+
+    const char *const promiser = P.currentpromise->promiser;
+    const char *const file = P.filename;
+    const char *const bundle = P.currentbundle->name;
+    size_t line = P.currentpromise->offset.line;
+    ParseWarning(PARSER_WARNING_SANITY_CHECK,
+        "No action requested for %s promise with promiser '%s' in %s:%s close to line %zu",
+        promise_type, promiser, file, bundle, line);
+}
+
 #endif // CF3_PARSE_LOGIC_H

libpromises/parser.c

@@ -164,6 +164,10 @@ int ParserWarningFromString(const char *warning_str)
     {
         return PARSER_WARNING_REMOVED;
     }
+    else if (strcmp("sanity-check", warning_str) == 0)
+    {
+        return PARSER_WARNING_SANITY_CHECK;
+    }
     else if (strcmp("all", warning_str) == 0)
     {
         return PARSER_WARNING_ALL;
@@ -182,6 +186,8 @@ const char *ParserWarningToString(unsigned int warning)
         return "deprecated";
     case PARSER_WARNING_REMOVED:
         return "removed";
+    case PARSER_WARNING_SANITY_CHECK:
+        return "sanity-check";
 
     default:
         ProgrammingError("Invalid parser warning: %u", warning);

libpromises/parser.h

@@ -29,6 +29,7 @@
 
 #define PARSER_WARNING_DEPRECATED       (1 << 0)
 #define PARSER_WARNING_REMOVED          (1 << 1)
+#define PARSER_WARNING_SANITY_CHECK     (1 << 2)
 
 #define PARSER_WARNING_ALL              0xfffffff
 

libpromises/policy.c

@@ -2328,7 +2328,7 @@ static Body *PolicyAppendBodyJson(Policy *policy, JsonElement *json_body)
     }
 
     Body *body = PolicyAppendBody(policy, ns, name, type, args, source_path, false);
-
+    RlistDestroy(args); // It's copied by PolicyAppendBody()
     {
         JsonElement *json_contexts = JsonObjectGetAsArray(json_body, "contexts");
         for (size_t i = 0; i < JsonLength(json_contexts); i++)

tests/unit/data/benchmark.cf

@@ -22,7 +22,14 @@ bundle agent main
       perms => myperms;
 
   processes:
-      "/bin/stuff" -> { "stakeholder" };
+      "/bin/stuff" -> { "stakeholder" }
+      process_count => any_count("stuff_running");
+}
+
+body process_count any_count(cl)
+{
+      match_range => "0,0";
+      out_of_range_define => { "$(cl)" };
 }
 
 body perms myperms

tests/unit/policy_test.c

@@ -174,7 +174,7 @@ static void test_policy_json_to_from(void)
     assert_true(policy);
 
     assert_int_equal(1, SeqLength(policy->bundles));
-    assert_int_equal(2, SeqLength(policy->bodies));
+    assert_int_equal(3, SeqLength(policy->bodies));
 
     {
         Bundle *main_bundle = PolicyGetBundle(policy, NULL, "agent", "main");
@@ -320,15 +320,15 @@ static void test_policy_json_offsets(void)
 
         JsonElement *myperms_body = JsonArrayGetAsObject(json_bodies, 1);
         line = JsonPrimitiveGetAsInteger(JsonObjectGet(myperms_body, "line"));
-        assert_int_equal(28, line);
+        assert_int_equal(29, line);
 
         JsonElement *myperms_contexts = JsonObjectGetAsArray(myperms_body, "contexts");
         JsonElement *any_context = JsonArrayGetAsObject(myperms_contexts, 0);
         JsonElement *any_attribs = JsonObjectGetAsArray(any_context, "attributes");
         {
             JsonElement *mode_attrib = JsonArrayGetAsObject(any_attribs, 0);
             line = JsonPrimitiveGetAsInteger(JsonObjectGet(mode_attrib, "line"));
-            assert_int_equal(30, line);
+            assert_int_equal(31, line);
         }
     }