Skip to content

Commit

Permalink
Fix strings passed to Check's asserts in tests
Browse files Browse the repository at this point in the history
There were cases where Check's unit tests were using Check
asserts by passing in a message that was either not a string
literal or passed no format arguments. This was pointed out
by the format-security warning. This commit fixes these cases.
  • Loading branch information
brarcher committed Jun 28, 2020
1 parent 4abf87c commit 17fdbf2
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 73 deletions.
12 changes: 6 additions & 6 deletions tests/check_check_fixture.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
66 changes: 4 additions & 62 deletions tests/check_check_master.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down
10 changes: 5 additions & 5 deletions tests/check_check_pack.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 17fdbf2

Please sign in to comment.