test: avoid double evaluation in ASSERT_BASE macro

Passing expression as an argument to a function-like macro will replace
all occurrence of the arguments with expressions during preprocessing.
This result in multiple evaluation of the same expression and can
slow-down the program or even change program state. Here ASSERT_BASE
macro gets an expression involving a and b as first argument and macro
definition has a print statement with a and b, which means there is
double evaluation of a and b when the expression evaluates to false. To
avoid double evaluation temporary variables are created to store results
of a and b.

Since the expression argument is dropped from ASSERT_BASE, the macro no
longer works for string assertions. So a new macro, ASSERT_BASE_STR, is
introduced to deal with strings. ASSERT_BASE can still work with
pointers.

Fixes: https://github.com/libuv/libuv/issues/2916
PR-URL: https://github.com/libuv/libuv/pull/2926
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jameson Nash <vtjnash@gmail.com>

CMakeLists.txt

@@ -492,6 +492,7 @@ if(LIBUV_BUILD_TESTS)
        test/test-tcp-write-queue-order.c
        test/test-tcp-write-to-half-open-connection.c
        test/test-tcp-writealot.c
+       test/test-test-macros.c
        test/test-thread-equal.c
        test/test-thread.c
        test/test-threadpool-cancel.c

Makefile.am

@@ -274,6 +274,7 @@ test_run_tests_SOURCES = test/blackhole-server.c \
                          test/test-tcp-try-write.c \
                          test/test-tcp-try-write-error.c \
                          test/test-tcp-write-queue-order.c \
+                         test/test-test-macros.c \
                          test/test-thread-equal.c \
                          test/test-thread.c \
                          test/test-threadpool-cancel.c \

test/task.h

@@ -111,24 +111,44 @@ typedef enum {
   }                                                       \
  } while (0)
 
-#define ASSERT_BASE(expr, a, operator, b, type, conv)        \
+#define ASSERT_BASE(a, operator, b, type, conv)              \
  do {                                                        \
-  if (!(expr)) {                                             \
+  type eval_a = (type) (a);                                  \
+  type eval_b = (type) (b);                                  \
+  if (!(eval_a operator eval_b)) {                           \
     fprintf(stderr,                                          \
             "Assertion failed in %s on line %d: `%s %s %s` " \
-            "(%"conv" %s %"conv")\n",                      \
+            "(%"conv" %s %"conv")\n",                        \
             __FILE__,                                        \
             __LINE__,                                        \
             #a,                                              \
             #operator,                                       \
             #b,                                              \
-            (type)a,                                         \
+            eval_a,                                          \
             #operator,                                       \
-            (type)b);                                        \
+            eval_b);                                         \
     abort();                                                 \
   }                                                          \
  } while (0)
 
+#define ASSERT_BASE_STR(expr, a, operator, b, type, conv)      \
+ do {                                                          \
+  if (!(expr)) {                                               \
+    fprintf(stderr,                                            \
+            "Assertion failed in %s on line %d: `%s %s %s` "   \
+            "(%"conv" %s %"conv")\n",                          \
+            __FILE__,                                          \
+            __LINE__,                                          \
+            #a,                                                \
+            #operator,                                         \
+            #b,                                                \
+            (type)a,                                           \
+            #operator,                                         \
+            (type)b);                                          \
+    abort();                                                   \
+  }                                                            \
+ } while (0)
+
 #define ASSERT_BASE_LEN(expr, a, operator, b, conv, len)     \
  do {                                                        \
   if (!(expr)) {                                             \
@@ -177,7 +197,7 @@ typedef enum {
  } while (0)
 
 #define ASSERT_INT_BASE(a, operator, b, type, conv)          \
- ASSERT_BASE(a operator b, a, operator, b, type, conv)
+ ASSERT_BASE(a, operator, b, type, conv)
 
 #define ASSERT_EQ(a, b) ASSERT_INT_BASE(a, ==, b, int64_t, PRId64)
 #define ASSERT_GE(a, b) ASSERT_INT_BASE(a, >=, b, int64_t, PRId64)
@@ -194,10 +214,10 @@ typedef enum {
 #define ASSERT_UINT64_NE(a, b) ASSERT_INT_BASE(a, !=, b, uint64_t, PRIu64)
 
 #define ASSERT_STR_EQ(a, b) \
-  ASSERT_BASE(strcmp(a, b) == 0, a, ==, b, char*, "s")
+  ASSERT_BASE_STR(strcmp(a, b) == 0, a, == , b, char*, "s")
 
 #define ASSERT_STR_NE(a, b) \
-  ASSERT_BASE(strcmp(a, b) != 0, a, !=, b, char*, "s")
+  ASSERT_BASE_STR(strcmp(a, b) != 0, a, !=, b, char*, "s")
 
 #define ASSERT_MEM_EQ(a, b, size) \
   ASSERT_BASE_LEN(memcmp(a, b, size) == 0, a, ==, b, s, size)
@@ -212,16 +232,16 @@ typedef enum {
   ASSERT_BASE_HEX(memcmp(a, b, size) != 0, a, !=, b, size)
 
 #define ASSERT_NULL(a) \
-  ASSERT_BASE(a == NULL, a, ==, NULL, void*, "p")
+  ASSERT_BASE(a, ==, NULL, void*, "p")
 
 #define ASSERT_NOT_NULL(a) \
-  ASSERT_BASE(a != NULL, a, !=, NULL, void*, "p")
+  ASSERT_BASE(a, !=, NULL, void*, "p")
 
 #define ASSERT_PTR_EQ(a, b) \
-  ASSERT_BASE((void*)a == (void*)b, a, ==, b, void*, "p")
+  ASSERT_BASE(a, ==, b, void*, "p")
 
 #define ASSERT_PTR_NE(a, b) \
-  ASSERT_BASE((void*)a != (void*)b, a, !=, b, void*, "p")
+  ASSERT_BASE(a, !=, b, void*, "p")
 
 /* This macro cleans up the main loop. This is used to avoid valgrind
  * warnings about memory being "leaked" by the main event loop.

test/test-list.h

@@ -283,6 +283,7 @@ TEST_DECLARE   (getnameinfo_basic_ip6)
 TEST_DECLARE   (getsockname_tcp)
 TEST_DECLARE   (getsockname_udp)
 TEST_DECLARE   (gettimeofday)
+TEST_DECLARE   (test_macros)
 TEST_DECLARE   (fail_always)
 TEST_DECLARE   (pass_always)
 TEST_DECLARE   (socket_buffer_size)
@@ -534,6 +535,7 @@ TASK_LIST_START
 #if 0
   TEST_ENTRY  (callback_order)
 #endif
+  TEST_ENTRY  (test_macros)
   TEST_ENTRY  (close_order)
   TEST_ENTRY  (run_once)
   TEST_ENTRY  (run_nowait)

test/test-test-macros.c

@@ -0,0 +1,42 @@
+/* Copyright libuv contributors. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "task.h"
+
+int test_macros_evil(void) {
+  static int x;
+  return x++;
+}
+
+
+TEST_IMPL(test_macros) {
+  char* a = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+  char* b = "ABCDEFGHIJKLMNOPQRSTUVWXYz";
+  char* c = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+  int i;
+
+  i = test_macros_evil();
+  ASSERT_STR_NE(a, b);
+  ASSERT_STR_EQ(a, c);
+  ASSERT_EQ(i + 1, test_macros_evil());
+  ASSERT_EQ(i + 2, test_macros_evil());
+  return 0;
+}