Merge pull request #283 from libcheck/format-security-in-tests

Fix strings passed to Check's asserts in tests
libcheck · Jun 28, 2020 · ab0c09b · ab0c09b
2 parents 4abf87c + 17fdbf2
commit ab0c09b
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 73 deletions.
diff --git a/tests/check_check_fixture.c b/tests/check_check_fixture.c
@@ -93,7 +93,7 @@ START_TEST(test_setup_failure_msg)
     snprintf(errm, sizeof(errm),
 	     "Bad setup tr msg (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
 }
@@ -217,7 +217,7 @@ START_TEST(test_ch_setup_fail)
     snprintf(errm, sizeof(errm),
 	     "Bad failed checked setup tr msg (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
   free(tr);
@@ -348,7 +348,7 @@ START_TEST(test_ch_setup_sig)
     snprintf(errm, sizeof(errm),
 	     "Msg was (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
   srunner_free(sr);
@@ -440,7 +440,7 @@ START_TEST(test_ch_teardown_fail)
     snprintf(errm, sizeof(errm),
 	     "Bad failed checked teardown tr msg (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
   free(tr);
@@ -486,7 +486,7 @@ START_TEST(test_ch_teardown_fail_nofork)
     snprintf(errm, sizeof(errm),
 	     "Bad failed checked teardown tr msg (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
   free(tr);
@@ -542,7 +542,7 @@ START_TEST(test_ch_teardown_sig)
     snprintf(errm, sizeof(errm),
 	     "Bad msg (%s)", trm);
 
-    ck_abort_msg (errm);
+    ck_abort_msg("%s", errm);
   }
   free(trm);
   srunner_free(sr);

diff --git a/tests/check_check_master.c b/tests/check_check_master.c
@@ -436,36 +436,6 @@ START_TEST(test_check_ntests_run)
 }
 END_TEST
 
-/**
- * Given a string, return a new string that is a copy
- * of the original exception that every occurance of
- * % is replaced with %%. This escapes the %
- * symbol for passing to printf.
- *
- * The passed in string is not modified. Note though
- * that the returned string is allocated memory that
- * must be freed by the caller.
- */
-char * escape_percent(const char *original, size_t original_size);
-char * escape_percent(const char *original, size_t original_size)
-{
-  /* In the worst case every character is a %*/
-  char *result = (char*)malloc(original_size*2);
-
-  size_t read_index;
-  size_t write_index;
-  for(read_index = write_index = 0; read_index < original_size; read_index++, write_index++)
-  {
-    result[write_index] = original[read_index];
-    if(result[write_index] == '%')
-    {
-      /* Place a duplicate % next to the one just read, to escape it */
-      result[++write_index] = '%';
-    }
-  }
-
-  return result;
-}
 
 START_TEST(test_check_failure_msgs)
 {
@@ -474,9 +444,7 @@ START_TEST(test_check_failure_msgs)
   const char *got_msg;
   const char *expected_msg;
   unsigned char not_equal = 0;
-  char emsg[MAXSTR];
   const char *msg_type_str;
-  char *emsg_escaped;
   int reg_err;
   char err_text[256];
   TestResult *tr;
@@ -538,21 +506,9 @@ START_TEST(test_check_failure_msgs)
         msg_type_str = "";
       }
 
-      snprintf(emsg, MAXSTR - 1,"For test %d:%s:%s Expected%s '%s', got '%s'",
+      ck_abort_msg("For test %d:%s:%s Expected%s '%s', got '%s'",
                i, master_test->tcname, master_test->test_name, msg_type_str,
                expected_msg, got_msg);
-      emsg[MAXSTR - 1] = '\0';
-
-      /*
-       * NOTE: ck_abort_msg() will take the passed string
-       * and feed it to printf. We need to escape any
-       * '%' found, else they will result in odd formatting
-       * in ck_abort_msg().
-       */
-      emsg_escaped = escape_percent(emsg, MAXSTR);
-
-      ck_abort_msg(emsg_escaped);
-      free(emsg_escaped);
     }
   }
 }
@@ -714,8 +670,6 @@ START_TEST(test_check_all_msgs)
 
   if (not_equal) {    
     const char *msg_type_str;
-    char emsg[MAXSTR];
-    char *emsg_escaped;
     switch(master_test->msg_type) {
 #if ENABLE_REGEX
     case CK_MSG_REGEXP:
@@ -726,21 +680,9 @@ START_TEST(test_check_all_msgs)
       msg_type_str = "";
     }
 
-    snprintf(emsg, MAXSTR - 1, "For test %i:%s:%s expected%s '%s', got '%s'",
-             _i, master_test->tcname, master_test->test_name, msg_type_str,
-             expected_msg, got_msg);
-    emsg[MAXSTR - 1] = '\0';
-
-   /*
-    * NOTE: ck_abort_msg() will take the passed string
-    * and feed it to printf. We need to escape any
-    * '%' found, else they will result in odd formatting
-    * in ck_abort_msg().
-    */
-    emsg_escaped = escape_percent(emsg, MAXSTR);
-
-    ck_abort_msg(emsg_escaped);
-    free(emsg_escaped);
+    ck_abort_msg("For test %i:%s:%s expected%s '%s', got '%s'",
+       _i, master_test->tcname, master_test->test_name, msg_type_str,
+       expected_msg, got_msg);
   }
 }
 END_TEST  

diff --git a/tests/check_check_pack.c b/tests/check_check_pack.c
@@ -57,7 +57,7 @@ START_TEST(test_pack_fmsg)
     snprintf (errm, sizeof(errm),
 	      "Unpacked string is %s, should be Hello, World!",
 	      fmsg->msg);
-    fail (errm);
+    fail("%s", errm);
   }
 
   free (fmsg->msg);
@@ -88,14 +88,14 @@ START_TEST(test_pack_loc)
     snprintf (errm, sizeof (errm),
 	     "LocMsg line was %d, should be %d",
 	     lmsg->line, 125);
-    fail (errm);
+    fail("%s", errm);
   }
 
   if (strcmp (lmsg->file, "abc123.c") != 0) {
     snprintf (errm, sizeof (errm),
               "LocMsg file was %s, should be abc123.c",
               lmsg->file);
-    fail (errm);
+    fail("%s", errm);
   }
 
   free (lmsg->file);
@@ -123,7 +123,7 @@ START_TEST(test_pack_ctx)
     snprintf (errm, sizeof (errm),
 	     "CtxMsg ctx got %d, expected %d",
 	     cmsg.ctx, CK_CTX_SETUP);
-    fail (errm);
+    fail("%s", errm);
   }
 
   free (buf);
@@ -147,7 +147,7 @@ START_TEST(test_pack_len)
   n = upack (buf, (CheckMsg *) &cmsg, &type);
   if (n != 8) {
     snprintf (errm, sizeof (errm), "%d bytes read from upack, should be 8", n);
-    fail (errm);
+    fail("%s", errm);
   }
 
   free (buf);