diff options
| author | Scott Gasch <[email protected]> | 2026-10-15 16:26:43 -0700 |
|---|---|---|
| committer | Scott Gasch <[email protected]> | 2026-10-15 16:26:43 -0700 |
| commit | c179ae5ad9f1a0efb68904033621705fd060aea4 (patch) | |
| tree | 2f0639b4699a7825b6983f27518497d388eb0e2d /src | |
| parent | 1ea81bae240912efb8cebabae35fe58bc5d12afb (diff) | |
Fix overlapping-buffer sprintf/snprintf bugs in fen.c and gamelist.c
Both FenFromPosition and the status-line builder in gamelist.c were
reading and writing the same buffer within a single sprintf/snprintf
call (e.g. sprintf(pHeap, "%s...", pHeap, ...)) -- undefined behavior
per the C standard since the source and destination overlap, caught
by valgrind as uninitialised-value reads even though no DEBUG ASSERT
ever tripped. Fixed by building each piece into a separate buffer (or
appending via snprintf/strncat into the already-terminated prefix)
instead of re-reading the destination as a source argument.
debug_smoke_test.sh: added a valgrind pass over the same sampled
smoke-test positions, since this class of bug produces no ASSERT
failure and was otherwise invisible to the existing smoke test.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Diffstat (limited to 'src')
| -rwxr-xr-x | src/debug_smoke_test.sh | 26 | ||||
| -rw-r--r-- | src/fen.c | 17 | ||||
| -rw-r--r-- | src/gamelist.c | 19 |
3 files changed, 46 insertions, 16 deletions
diff --git a/src/debug_smoke_test.sh b/src/debug_smoke_test.sh index d8ef228..f1ce4e4 100755 --- a/src/debug_smoke_test.sh +++ b/src/debug_smoke_test.sh @@ -80,3 +80,29 @@ if [ "$STATUS" -ne 0 ]; then exit 1 fi echo "SMOKE TEST PASSED" + +# Valgrind pass: catches memory errors (e.g. overlapping-buffer sprintf/ +# memcpy, invalid reads/writes) that don't trip a DEBUG ASSERT but are +# still real bugs. Reuses the same sampled positions. --cpus 1 because +# valgrind + MP threading is slow and noisy for no real benefit here -- +# this is checking memory safety, not search behavior. error-exitcode=99 +# distinguishes "valgrind found a problem" from a normal engine exit. +if command -v valgrind >/dev/null 2>&1; then + VGLOG="$LOGDIR/valgrind_${STAMP}.log" + VGOUT="$LOGDIR/valgrind_${STAMP}.out" + echo "=== valgrind pass over the same $COUNT positions ===" + valgrind -s --leak-check=full --error-exitcode=99 --log-file="$VGLOG" \ + "$BIN" --cpus 1 --hash 64m --logfile /dev/null \ + --batch --command "force; book name /nonexistent.book.bin; sd $DEPTH; script $SAMPLE" \ + > "$VGOUT" 2>&1 + VGSTATUS=$? + if [ "$VGSTATUS" -eq 99 ]; then + echo " *** VALGRIND ERRORS DETECTED -- see $VGLOG ***" + echo "SMOKE TEST FAILED" + exit 1 + fi + echo " valgrind: clean (see $VGLOG)" +else + echo " (valgrind not found on PATH, skipping memory-error pass)" +fi +echo "SMOKE TEST PASSED" @@ -188,15 +188,16 @@ Return value: // queenside. There is no white space between the letters. // *pch++ = ' '; - *pch++ = '\0'; + *pch = '\0'; + ASSERT(strlen(pHeap) < MAX_FEN_LEN_BYTES); + snprintf(pch, + MAX_FEN_LEN_BYTES - (size_t)(pch - pHeap), + "%s %s %u %u", + CastleInfoString(pos->bvCastleInfo), + CoorToString(pos->cEpSquare), + pos->uFifty, + 0); ASSERT(strlen(pHeap) < MAX_FEN_LEN_BYTES); - sprintf(pHeap, - "%s%s %s %u %u", - pHeap, - CastleInfoString(pos->bvCastleInfo), - CoorToString(pos->cEpSquare), - pos->uFifty, - 0); return(pHeap); // Note: caller must free } diff --git a/src/gamelist.c b/src/gamelist.c index a409f07..0a0bdd1 100644 --- a/src/gamelist.c +++ b/src/gamelist.c @@ -1099,23 +1099,26 @@ Return value: char buf[SMALL_STRING_LEN_CHAR]; ASSERT(IS_VALID_COLOR(g_uToMove)); - if (g_Options.fStatusLine) + if (g_Options.fStatusLine) { strcpy(buf, "_\0"); - if (!g_Options.fRunningUnderXboard) + if (!g_Options.fRunningUnderXboard) { if ((TRUE == g_Options.fPondering) && (g_Options.mvPonder.uMove != 0)) { snprintf(buf, SMALL_STRING_LEN_CHAR - 1, - "_[pondering %s] ", + "_[pondering %s] ", MoveToSan(g_Options.mvPonder, &g_RootPosition)); } - snprintf(buf, SMALL_STRING_LEN_CHAR - strlen(buf), - "%s%s(move %u)_", - buf, - (g_uToMove == WHITE) ? "white" : "black", - g_uMoveNumber[g_uToMove]); + { + char bufTail[SMALL_STRING_LEN_CHAR]; + snprintf(bufTail, sizeof(bufTail), + "%s(move %u)_", + (g_uToMove == WHITE) ? "white" : "black", + g_uMoveNumber[g_uToMove]); + strncat(buf, bufTail, SMALL_STRING_LEN_CHAR - strlen(buf) - 1); + } Trace("%s\n", buf); } } |
