From feb6f8583f18ee274ff7fbd24ed6a96f9c5b638d Mon Sep 17 00:00:00 2001 From: "B. Scott Michel" Date: Wed, 29 Nov 2023 13:10:58 -0800 Subject: [PATCH] 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. --- scp.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----- sim_sock.c | 2 +- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/scp.c b/scp.c index 7acd1f194..002a3b36d 100644 --- a/scp.c +++ b/scp.c @@ -335,6 +335,7 @@ #define SIM_DBG_DO 0x01000000 /* do activities */ #define SIM_DBG_SAVE 0x00800000 /* save activities */ #define SIM_DBG_RESTORE 0x00400000 /* restore activities */ +#define SCP_LOG_TESTING 0x00200000 /* test_scp_debug_logging() debug */ static DEBTAB scp_debug[] = { {"EVENT", SIM_DBG_EVENT, "Event Dispatch Activities"}, @@ -347,6 +348,7 @@ static DEBTAB scp_debug[] = { {"DO", SIM_DBG_DO, "Do Command/Expansion Activities"}, {"SAVE", SIM_DBG_SAVE, "Save Activities"}, {"RESTORE", SIM_DBG_RESTORE, "Restore Activities"}, + {"LOG TEST", SCP_LOG_TESTING, "Log testing"}, {0} }; @@ -13560,6 +13562,7 @@ const char *debug_bstates = "01_^"; AIO_TLS char debug_line_prefix[256]; int32 debug_unterm = 0; char *debug_line_buf_last = NULL; +size_t debug_line_buf_last_len = 0; size_t debug_line_buf_last_endprefix_offset = 0; AIO_TLS char debug_line_last_prefix[256]; char *debug_line_buf = NULL; @@ -13622,15 +13625,25 @@ if (sim_deb_switches & SWMASK ('F')) { /* filtering disabled? */ } AIO_LOCK; if (debug_line_offset + len + 1 > debug_line_bufsize) { + /* realloc(NULL, size) == malloc(size). Initialize the malloc()-ed space. Only + need to test debug_line_buf since SIMH allocates both buffers at the same + time. */ + const int do_init = (NULL == debug_line_buf); + debug_line_bufsize += MAX(1024, debug_line_offset + len + 1); debug_line_buf = (char *)realloc (debug_line_buf, debug_line_bufsize); debug_line_buf_last = (char *)realloc (debug_line_buf_last, debug_line_bufsize); + + if (do_init) { + memset(debug_line_buf, 0, debug_line_bufsize); + memset(debug_line_buf_last, 0, debug_line_bufsize); + } } memcpy (&debug_line_buf[debug_line_offset], buf, len); debug_line_buf[debug_line_offset + len] = '\0'; debug_line_offset += len; -while ((eol = strchr (debug_line_buf, '\n')) || flush) { - char *endprefix = strstr (debug_line_buf, ")> "); +while (NULL != (eol = strchr (debug_line_buf, '\n')) || flush) { + const char *endprefix = strstr (debug_line_buf, ")> "); size_t linesize = (eol - debug_line_buf) + 1; if ((0 != memcmp ("DBG(", debug_line_buf, 4)) || (endprefix == NULL)) { @@ -13657,12 +13670,18 @@ while ((eol = strchr (debug_line_buf, '\n')) || flush) { debug_line_buf_last_endprefix_offset = endprefix - debug_line_buf; memcpy (debug_line_buf_last, debug_line_buf, linesize); debug_line_buf_last[linesize] = '\0'; + debug_line_buf_last_len = (NULL != eol) ? eol - endprefix + 1 : linesize; debug_line_count = 1; } else { - if (0 == memcmp (&debug_line_buf[endprefix - debug_line_buf], - &debug_line_buf_last[debug_line_buf_last_endprefix_offset], - (eol - endprefix)+ 1)) { + const size_t compare_len = eol - endprefix + 1; + + /* Ensure SIMH only executes memcmp() if the last message's comparison length + is the same as the current debug line's comparison length. */ + if (debug_line_buf_last_len == compare_len && + 0 == memcmp (&debug_line_buf[endprefix - debug_line_buf], + &debug_line_buf_last[debug_line_buf_last_endprefix_offset], + compare_len)) { ++debug_line_count; memcpy (debug_line_last_prefix, debug_line_buf, (endprefix - debug_line_buf) + 3); debug_line_last_prefix[(endprefix - debug_line_buf) + 3] = '\0'; @@ -13679,6 +13698,7 @@ while ((eol = strchr (debug_line_buf, '\n')) || flush) { debug_line_buf_last_endprefix_offset = endprefix - debug_line_buf; memcpy (debug_line_buf_last, debug_line_buf, linesize); debug_line_buf_last[linesize] = '\0'; + debug_line_buf_last_len = (NULL != eol) ? compare_len : linesize; debug_line_count = 1; } } @@ -16442,6 +16462,36 @@ sim_set_deb_switches (start_deb_switches); return r; } +static t_stat test_scp_debug_logging() +{ +uint32 saved_scp_dev_dbits = sim_scp_dev.dctrl; +FILE *saved_sim_deb = sim_deb; + +sim_scp_dev.dctrl = SCP_LOG_TESTING; +sim_deb = stderr; + +_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n"); +_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n"); +_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n"); +_sim_debug_device (SCP_LOG_TESTING, &sim_scp_dev, "Log message (repeats 3x)\n"); + +if (debug_line_count != 4) + return sim_messagef(SCPE_IERR, "expected debug_line_count == 4, actual %d\n", (int) debug_line_count); + +_sim_debug_device (0xffffffff, &sim_scp_dev, "Different message.\n"); +_sim_debug_flush (); + +if (debug_line_count > 0) + return sim_messagef(SCPE_IERR, "expected debug_line_count == 0, actual %d\n", (int) debug_line_count); + +sim_deb = saved_sim_deb; +sim_scp_dev.dctrl = saved_scp_dev_dbits; + +sim_printf ("Log de-duplication successful.\n"); + +return SCPE_OK; +} + /* * Compiled in unit tests for the various device oriented library * 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)) { return sim_messagef (SCPE_IERR, "SCP argument parsing test failed\n"); if (test_scp_event_sequencing () != SCPE_OK) return sim_messagef (SCPE_IERR, "SCP event sequencing test failed\n"); + if (test_scp_debug_logging () != SCPE_OK) + return sim_messagef (SCPE_IERR, "SCP debug logging test failed\n"); } for (i = 0; (dptr = sim_devices[i]) != NULL; i++) { t_stat tstat = SCPE_OK; diff --git a/sim_sock.c b/sim_sock.c index 1d834ab83..cd67895c3 100644 --- a/sim_sock.c +++ b/sim_sock.c @@ -721,7 +721,7 @@ if (c != NULL) { if ((c - validate_addr) > sizeof (v_cpy) - 1) return status; memcpy (v_cpy, validate_addr, c - validate_addr); /* Copy everything before the / */ - v_cpy[1 + c - validate_addr] = '\0'; /* NUL terminate the result */ + v_cpy[c - validate_addr] = '\0'; /* NUL terminate the result */ validate_addr = v_cpy; /* Use the original string minus the prefix specifier */ } if (p_getaddrinfo(validate_addr, NULL, NULL, &ai_validate))