From d5a7372bcef9a63dd53f58bd3951f8392ad21e00 Mon Sep 17 00:00:00 2001
From: Alexis Mousset <alexis.mousset@rudder.io>
Date: Thu, 31 Aug 2023 17:53:09 +0200
Subject: [PATCH] Fix RlistEqual comparison on lists of different lengths

The original issue that leads to this is the fact that the cache
for execresult and execresult_as_data were mixed. This is caused by two
separate issues:

* The function cache uses the args list as key and discards the function
  itself. This means different function with the same args are considered
  identical, and cache is reused.
* The args are passed as an Rlist, and the used Rlist comparison ignores the
  additional items of the longest list when comparing two lists of
  different lengths, leading to treating execresult and execresult_as_data
  as identical when using the same command and shell args.

This PR only fixes the specific case of execresult, but leaves other
function cache issues (e.g. host2ip vs. ip2host could be confused).
---
 libpromises/rlist.c                           |  4 +-
 .../04_containers/execresult_and_as_data.cf   | 41 +++++++++++++++++++
 tests/unit/rlist_test.c                       | 18 ++++++++
 3 files changed, 61 insertions(+), 2 deletions(-)
 create mode 100644 tests/acceptance/01_vars/04_containers/execresult_and_as_data.cf

diff --git a/libpromises/rlist.c b/libpromises/rlist.c
index 909ef7633f..d98c1063ad 100644
--- a/libpromises/rlist.c
+++ b/libpromises/rlist.c
@@ -1657,8 +1657,8 @@ bool RlistEqual(const Rlist *list1, const Rlist *list2)
             assert(rp1->val.item == NULL && rp2->val.item == NULL);
         }
     }
-
-    return true;
+    // return false if lengths are different
+    return (rp1 == NULL && rp2 == NULL);
 }
 
 bool RlistEqual_untyped(const void *list1, const void *list2)
diff --git a/tests/acceptance/01_vars/04_containers/execresult_and_as_data.cf b/tests/acceptance/01_vars/04_containers/execresult_and_as_data.cf
new file mode 100644
index 0000000000..8404a26236
--- /dev/null
+++ b/tests/acceptance/01_vars/04_containers/execresult_and_as_data.cf
@@ -0,0 +1,41 @@
+#######################################################
+#
+# Test the ability to call the same command with execresult and execresult_as_data
+#
+#######################################################
+
+body common control
+{
+      inputs => { "../../default.cf.sub" };
+      bundlesequence  => { default("$(this.promise_filename)") };
+      version => "1.0";
+}
+
+#######################################################
+
+
+bundle agent init
+{
+}
+
+#######################################################
+
+bundle agent test
+{
+  vars:
+    !windows::
+     "res1" string => execresult("echo test", "useshell");
+     "res2" data   => execresult_as_data("echo test", "useshell", "stdout");
+    windows::
+     "res1" string => execresult("echo test", "powershell");
+     "res2" data   => execresult_as_data("echo test", "powershell", "stdout");
+}
+
+
+#######################################################
+
+bundle agent check
+{
+  methods:
+      "any" usebundle => dcs_check_strcmp("${test.res1}", "${test.res2[output]}", "$(this.promise_filename)", "no");
+}
diff --git a/tests/unit/rlist_test.c b/tests/unit/rlist_test.c
index e713368ad9..5064fe08a7 100644
--- a/tests/unit/rlist_test.c
+++ b/tests/unit/rlist_test.c
@@ -31,6 +31,23 @@ static void test_length(void)
     RlistDestroy(list);
 }
 
+static void test_equality(void)
+{
+    Rlist *list1 = RlistFromSplitString("a,b,c", ',');
+    Rlist *list2 = RlistFromSplitString("a,b,c", ',');
+    Rlist *list3 = RlistFromSplitString("z,b,c", ',');
+    Rlist *list4 = RlistFromSplitString("a,b,c,d", ',');
+
+    assert_true(RlistEqual(list1, list2));
+    assert_false(RlistEqual(list1, list3));
+    assert_false(RlistEqual(list1, list4));
+
+    RlistDestroy(list1);
+    RlistDestroy(list2);
+    RlistDestroy(list3);
+    RlistDestroy(list4);
+}
+
 static void test_prepend_scalar_idempotent(void)
 {
     Rlist *list = NULL;
@@ -750,6 +767,7 @@ int main()
     {
         unit_test(test_prepend_scalar_idempotent),
         unit_test(test_length),
+        unit_test(test_equality),
         unit_test(test_copy),
         unit_test(test_rval_to_scalar),
         unit_test(test_rval_to_scalar2),