Skip to content

Commit feb6f85

Browse files
committed
SCP: Add'l memory sanitization fixes
Initialize de-dup'ed debug line buffer: realloc(NULL, size) == malloc(size), which is uninitialized space. This causes the Clang memory sanitizer to detect an attempt to read uninitialized memory when debug_line_buf and debug_line_buf_last are different lengths. While the uninitialized space may never actually be compared, the memory sanitizer emits a strong hint to not do stupid. The sanitizer trips in the i650 simulator on the first memcmp(), debug_line_buf has 108 characters, debug_line_buf_last has 56 characters (uninitialized space follows the 56 characters, tripping the sanitizer.) - memset() debug_line_buf and debug_line_buf_last to zero so that memcmp() will always gracefully return non-zero if somehow memcmp() ends up going past the end of either buffer. Should never happen in practice, but theory always gets mugged by reality. - Keep track of debug_line_buf_last's comparison length (i.e., up to the '\r') and only execute memcmp() when this length equals the current debug_line_buf comparison length (end - endprefix + 1). - Added a log deduplication test to "testlib" command to ensure that nothing broke as a result of this fix. Network ACL check in sim_addr_acl_check: The memory sanitizer found an off-by-one bug in sim_addr_acl_check while executing "testlib". This makes CIDR network ACLs functional, e.g., "127.0.0.1/32" is interpreted properly and the associated "testlib" test passes.
1 parent 625b9e8 commit feb6f85

2 files changed

Lines changed: 58 additions & 6 deletions

File tree

scp.c

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@
335335
#define SIM_DBG_DO 0x01000000 /* do activities */
336336
#define SIM_DBG_SAVE 0x00800000 /* save activities */
337337
#define SIM_DBG_RESTORE 0x00400000 /* restore activities */
338+
#define SCP_LOG_TESTING 0x00200000 /* test_scp_debug_logging() debug */
338339

339340
static DEBTAB scp_debug[] = {
340341
{"EVENT", SIM_DBG_EVENT, "Event Dispatch Activities"},
@@ -347,6 +348,7 @@ static DEBTAB scp_debug[] = {
347348
{"DO", SIM_DBG_DO, "Do Command/Expansion Activities"},
348349
{"SAVE", SIM_DBG_SAVE, "Save Activities"},
349350
{"RESTORE", SIM_DBG_RESTORE, "Restore Activities"},
351+
{"LOG TEST", SCP_LOG_TESTING, "Log testing"},
350352
{0}
351353
};
352354

@@ -13560,6 +13562,7 @@ const char *debug_bstates = "01_^";
1356013562
AIO_TLS char debug_line_prefix[256];
1356113563
int32 debug_unterm = 0;
1356213564
char *debug_line_buf_last = NULL;
13565+
size_t debug_line_buf_last_len = 0;
1356313566
size_t debug_line_buf_last_endprefix_offset = 0;
1356413567
AIO_TLS char debug_line_last_prefix[256];
1356513568
char *debug_line_buf = NULL;
@@ -13622,15 +13625,25 @@ if (sim_deb_switches & SWMASK ('F')) { /* filtering disabled? */
1362213625
}
1362313626
AIO_LOCK;
1362413627
if (debug_line_offset + len + 1 > debug_line_bufsize) {
13628+
/* realloc(NULL, size) == malloc(size). Initialize the malloc()-ed space. Only
13629+
need to test debug_line_buf since SIMH allocates both buffers at the same
13630+
time. */
13631+
const int do_init = (NULL == debug_line_buf);
13632+
1362513633
debug_line_bufsize += MAX(1024, debug_line_offset + len + 1);
1362613634
debug_line_buf = (char *)realloc (debug_line_buf, debug_line_bufsize);
1362713635
debug_line_buf_last = (char *)realloc (debug_line_buf_last, debug_line_bufsize);
13636+
13637+
if (do_init) {
13638+
memset(debug_line_buf, 0, debug_line_bufsize);
13639+
memset(debug_line_buf_last, 0, debug_line_bufsize);
13640+
}
1362813641
}
1362913642
memcpy (&debug_line_buf[debug_line_offset], buf, len);
1363013643
debug_line_buf[debug_line_offset + len] = '\0';
1363113644
debug_line_offset += len;
13632-
while ((eol = strchr (debug_line_buf, '\n')) || flush) {
13633-
char *endprefix = strstr (debug_line_buf, ")> ");
13645+
while (NULL != (eol = strchr (debug_line_buf, '\n')) || flush) {
13646+
const char *endprefix = strstr (debug_line_buf, ")> ");
1363413647
size_t linesize = (eol - debug_line_buf) + 1;
1363513648

1363613649
if ((0 != memcmp ("DBG(", debug_line_buf, 4)) || (endprefix == NULL)) {
@@ -13657,12 +13670,18 @@ while ((eol = strchr (debug_line_buf, '\n')) || flush) {
1365713670
debug_line_buf_last_endprefix_offset = endprefix - debug_line_buf;
1365813671
memcpy (debug_line_buf_last, debug_line_buf, linesize);
1365913672
debug_line_buf_last[linesize] = '\0';
13673+
debug_line_buf_last_len = (NULL != eol) ? eol - endprefix + 1 : linesize;
1366013674
debug_line_count = 1;
1366113675
}
1366213676
else {
13663-
if (0 == memcmp (&debug_line_buf[endprefix - debug_line_buf],
13664-
&debug_line_buf_last[debug_line_buf_last_endprefix_offset],
13665-
(eol - endprefix)+ 1)) {
13677+
const size_t compare_len = eol - endprefix + 1;
13678+
13679+
/* Ensure SIMH only executes memcmp() if the last message's comparison length
13680+
is the same as the current debug line's comparison length. */
13681+
if (debug_line_buf_last_len == compare_len &&
13682+
0 == memcmp (&debug_line_buf[endprefix - debug_line_buf],
13683+
&debug_line_buf_last[debug_line_buf_last_endprefix_offset],
13684+
compare_len)) {
1366613685
++debug_line_count;
1366713686
memcpy (debug_line_last_prefix, debug_line_buf, (endprefix - debug_line_buf) + 3);
1366813687
debug_line_last_prefix[(endprefix - debug_line_buf) + 3] = '\0';
@@ -13679,6 +13698,7 @@ while ((eol = strchr (debug_line_buf, '\n')) || flush) {
1367913698
debug_line_buf_last_endprefix_offset = endprefix - debug_line_buf;
1368013699
memcpy (debug_line_buf_last, debug_line_buf, linesize);
1368113700
debug_line_buf_last[linesize] = '\0';
13701+
debug_line_buf_last_len = (NULL != eol) ? compare_len : linesize;
1368213702
debug_line_count = 1;
1368313703
}
1368413704
}
@@ -16442,6 +16462,36 @@ sim_set_deb_switches (start_deb_switches);
1644216462
return r;
1644316463
}
1644416464

16465+
static t_stat test_scp_debug_logging()
16466+
{
16467+
uint32 saved_scp_dev_dbits = sim_scp_dev.dctrl;
16468+
FILE *saved_sim_deb = sim_deb;
16469+
16470+
sim_scp_dev.dctrl = SCP_LOG_TESTING;
16471+
sim_deb = stderr;
16472+
16473+
_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n");
16474+
_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n");
16475+
_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n");
16476+
_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n");
16477+
16478+
if (debug_line_count != 4)
16479+
return sim_messagef(SCPE_IERR, "expected debug_line_count == 4, actual %d\n", (int) debug_line_count);
16480+
16481+
_sim_debug_device (0xffffffff, &sim_scp_dev, "Different message.\n");
16482+
_sim_debug_flush ();
16483+
16484+
if (debug_line_count > 0)
16485+
return sim_messagef(SCPE_IERR, "expected debug_line_count == 0, actual %d\n", (int) debug_line_count);
16486+
16487+
sim_deb = saved_sim_deb;
16488+
sim_scp_dev.dctrl = saved_scp_dev_dbits;
16489+
16490+
sim_printf ("Log de-duplication successful.\n");
16491+
16492+
return SCPE_OK;
16493+
}
16494+
1644516495
/*
1644616496
* Compiled in unit tests for the various device oriented library
1644716497
* modules: sim_card, sim_disk, sim_tape, sim_ether, sim_tmxr, etc.
@@ -16486,6 +16536,8 @@ if ((strcmp (gbuf, "ALL") == 0) || (strcmp (gbuf, "SCP") == 0)) {
1648616536
return sim_messagef (SCPE_IERR, "SCP argument parsing test failed\n");
1648716537
if (test_scp_event_sequencing () != SCPE_OK)
1648816538
return sim_messagef (SCPE_IERR, "SCP event sequencing test failed\n");
16539+
if (test_scp_debug_logging () != SCPE_OK)
16540+
return sim_messagef (SCPE_IERR, "SCP debug logging test failed\n");
1648916541
}
1649016542
for (i = 0; (dptr = sim_devices[i]) != NULL; i++) {
1649116543
t_stat tstat = SCPE_OK;

sim_sock.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,7 @@ if (c != NULL) {
721721
if ((c - validate_addr) > sizeof (v_cpy) - 1)
722722
return status;
723723
memcpy (v_cpy, validate_addr, c - validate_addr); /* Copy everything before the / */
724-
v_cpy[1 + c - validate_addr] = '\0'; /* NUL terminate the result */
724+
v_cpy[c - validate_addr] = '\0'; /* NUL terminate the result */
725725
validate_addr = v_cpy; /* Use the original string minus the prefix specifier */
726726
}
727727
if (p_getaddrinfo(validate_addr, NULL, NULL, &ai_validate))

0 commit comments

Comments
 (0)