From b73bcc5b1ec42917e1eea6cc1955d4dc90ba86d1 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Tue, 22 Sep 2026 20:16:21 +0200 Subject: [PATCH 1/2] Windows builds alias snprintf to a call that leaves no terminator htslib.h defined snprintf as _snprintf under _WIN32. Microsoft documents _snprintf as writing no terminating NUL when it truncates, so every raw snprintf in the engine produced an unterminated string on Windows, and only there. A user hit it through htsblk.msg[80]: the crawler aborted in htsparse.c reading a failure message that had filled the field. Drop the alias, route the writers of r.msg through htsblk_failf(), and add a test that fails on any macro renaming a C99 printf call, since a Linux build cannot see one. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsback.c | 21 ++++++------- src/htslib.c | 20 ++++--------- src/htslib.h | 2 +- tests/490_no-legacy-printf-alias.test | 43 +++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 25 deletions(-) create mode 100755 tests/490_no-legacy-printf-alias.test diff --git a/src/htsback.c b/src/htsback.c index 21c72e272..569ebdc4b 100644 --- a/src/htsback.c +++ b/src/htsback.c @@ -1101,15 +1101,16 @@ int back_finalize(httrackp * opt, cache_back * cache, struct_back * sback, strcpybuff(back[p].r.msg, "Empty compressed file"); } } else { - snprintf(back[p].r.msg, sizeof(back[p].r.msg), - "Open error when decompressing (can not create temporary file %s)", - back[p].tmpfile); + htsblk_failf(&back[p].r, + "Open error when decompressing (can not create " + "temporary file %s)", + back[p].tmpfile); back[p].tmpfile[0] = '\0'; back[p].r.statuscode = STATUSCODE_INVALID; } } else { - snprintf(back[p].r.msg, sizeof(back[p].r.msg), - "Open error when decompressing (can not generate a temporary file)"); + htsblk_failf(&back[p].r, "Open error when decompressing (can " + "not generate a temporary file)"); } } // fermer fichier sortie @@ -1188,11 +1189,11 @@ int back_finalize(httrackp * opt, cache_back * cache, struct_back * sback, a local write's errno behind, and 0 for a bad stream. */ if (errno != 0) back_report_write_failure(opt, &back[p]); - snprintf(back[p].r.msg, sizeof(back[p].r.msg), - codec == HTS_CODEC_UNSUPPORTED - ? "Unsupported Content-Encoding (%s)" - : "Error when decompressing (%s)", - back[p].r.contentencoding); + htsblk_failf(&back[p].r, + codec == HTS_CODEC_UNSUPPORTED + ? "Unsupported Content-Encoding (%s)" + : "Error when decompressing (%s)", + back[p].r.contentencoding); /* Drop the undecoded body so the writer can't commit the coded bytes as the page; url_sav is left untouched. */ if (!back[p].r.is_write) diff --git a/src/htslib.c b/src/htslib.c index 93c297d9d..294dbbd1f 100644 --- a/src/htslib.c +++ b/src/htslib.c @@ -2326,8 +2326,7 @@ T_SOC newhttp_addr(httrackp *opt, const char *_iadr, htsblk *retour, int port, // a port filter named; an empty "host:" just means the default (#614) if (a[1] != '\0' && !hts_parse_url_port(a + 1, &port)) { if (retour != NULL) { - snprintf(retour->msg, sizeof(retour->msg), "Invalid port: %s", - a + 1); + htsblk_failf(retour, "Invalid port: %s", a + 1); } return INVALID_SOCKET; } @@ -2355,13 +2354,7 @@ T_SOC newhttp_addr(httrackp *opt, const char *_iadr, htsblk *retour, int port, printf("erreur gethostbyname\n"); #endif if (retour != NULL) { -#ifdef _WIN32 - snprintf(retour->msg, sizeof(retour->msg), - "Unable to get server's address: %s", error); -#else - snprintf(retour->msg, sizeof(retour->msg), - "Unable to get server's address: %s", error); -#endif + htsblk_failf(retour, "Unable to get server's address: %s", error); } return INVALID_SOCKET; } @@ -2408,8 +2401,8 @@ T_SOC newhttp_addr(httrackp *opt, const char *_iadr, htsblk *retour, int port, &bind_addr, &error) == NULL || bind(soc, &SOCaddr_sockaddr(bind_addr), SOCaddr_size(bind_addr)) != 0) { - snprintf(retour->msg, sizeof(retour->msg), - "Unable to bind the specificied server address: %s", error); + htsblk_failf( + retour, "Unable to bind the specificied server address: %s", error); deletesoc(soc); return INVALID_SOCKET; } @@ -2430,9 +2423,8 @@ T_SOC newhttp_addr(httrackp *opt, const char *_iadr, htsblk *retour, int port, #endif char errbuf[HTS_STRERROR_SIZE]; - snprintf(retour->msg, sizeof(retour->msg), - "Non-blocking socket failed: %s", - hts_strerror(last_errno, errbuf, sizeof(errbuf))); + htsblk_failf(retour, "Non-blocking socket failed: %s", + hts_strerror(last_errno, errbuf, sizeof(errbuf))); deletesoc(soc); return INVALID_SOCKET; } diff --git a/src/htslib.h b/src/htslib.h index 269c36af6..417c56a9c 100644 --- a/src/htslib.h +++ b/src/htslib.h @@ -709,7 +709,7 @@ HTS_STATIC int strcmpnocase(const char *a, const char *b) { #ifdef _WIN32 #define strcasecmp(a,b) stricmp(a,b) #define strncasecmp(a,b,n) strnicmp(a,b,n) -#define snprintf _snprintf +/* No snprintf alias: _snprintf leaves a truncated string unterminated. */ #endif /* MSVC ships these POSIX functions under other names. Kept out of the installed diff --git a/tests/490_no-legacy-printf-alias.test b/tests/490_no-legacy-printf-alias.test new file mode 100755 index 000000000..e6e972d65 --- /dev/null +++ b/tests/490_no-legacy-printf-alias.test @@ -0,0 +1,43 @@ +#!/bin/bash +# +# No header may give a C99 printf call another implementation under its own +# name. htslib.h used to carry "#define snprintf _snprintf" under _WIN32, and +# msvcrt's _snprintf writes no terminating NUL when it truncates, so on Windows +# alone every unchecked snprintf in the engine left its destination NUL-less. +# That reached a user as an abort reading back an 80-byte htsblk.msg the engine +# had filled to capacity. Linux CI cannot see the alias, so this scan is the +# only guard. A bounded wrapper is fine when it has a name of its own, the way +# htssafe.h's sprintfbuff() does. + +set -euo pipefail + +# shellcheck source=tests/testlib.sh +. "${0%"${0##*/}"}./testlib.sh" + +top="${abs_top_srcdir:?not run under make check}" +test -f "$top/src/htslib.h" || fail "no $top/src/htslib.h" + +work=$(mktemp -d "${TMPDIR:-/tmp}/printfalias.XXXXXX") || fail "no tmpdir" +cleanup_push rm -rf "$work" + +alias_re='^[[:space:]]*#[[:space:]]*define[[:space:]]+(v?(f|s|sn)?printf|v?asprintf)[[:space:](]' + +sources() { # sources DIR -> the .c and .h files under it + find "$1" \( -name '*.c' -o -name '*.h' \) -print | LC_ALL=C sort +} + +sources "$top/src" >"$work/files" +test -s "$work/files" || fail "no sources found under $top/src" +xargs grep -nHE "$alias_re" <"$work/files" >"$work/found" || true + +# Control: the scan must fire on the line this test exists to keep out. +mkdir -p "$work/ctl" +printf '#ifdef _WIN32\n#define snprintf _snprintf\n#endif\n' >"$work/ctl/legacy.h" +sources "$work/ctl" | xargs grep -nHE "$alias_re" >"$work/control" || true +test -s "$work/control" || + fail "the scan cannot see a legacy alias, so a clean result proves nothing" + +test ! -s "$work/found" || + fail_dump "a C99 printf call is aliased to another implementation under its own name. Call the real one, or give the wrapper its own name" "$work/found" + +echo "sources scanned: $(lines_of "$work/files"), aliases: 0" From 97ade02cd6b8c5aaed448dc9b1ae5fbe480fda6b Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Tue, 22 Sep 2026 20:24:29 +0200 Subject: [PATCH 2/2] Renumber the test, floor its scan, and cover a compiler-set alias Review findings on this branch. Test 490 collided with the number PR #1743 claims, so the file moves to 491. Its scan reported a clean tree whatever the file list held, so it now floors the count. It also missed the alias handed to the compiler rather than written in a header, which is how a Visual Studio build would carry one, so it reads the project files too. The no-argument message in htsback.c goes back to snprintf: its text is 65 bytes, so it cannot fill the field and the conversion bought nothing. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsback.c | 4 +-- tests/490_no-legacy-printf-alias.test | 43 ---------------------- tests/491_no-legacy-printf-alias.test | 51 +++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 45 deletions(-) delete mode 100755 tests/490_no-legacy-printf-alias.test create mode 100755 tests/491_no-legacy-printf-alias.test diff --git a/src/htsback.c b/src/htsback.c index 569ebdc4b..718ec2641 100644 --- a/src/htsback.c +++ b/src/htsback.c @@ -1109,8 +1109,8 @@ int back_finalize(httrackp * opt, cache_back * cache, struct_back * sback, back[p].r.statuscode = STATUSCODE_INVALID; } } else { - htsblk_failf(&back[p].r, "Open error when decompressing (can " - "not generate a temporary file)"); + snprintf(back[p].r.msg, sizeof(back[p].r.msg), + "Open error when decompressing (can not generate a temporary file)"); } } // fermer fichier sortie diff --git a/tests/490_no-legacy-printf-alias.test b/tests/490_no-legacy-printf-alias.test deleted file mode 100755 index e6e972d65..000000000 --- a/tests/490_no-legacy-printf-alias.test +++ /dev/null @@ -1,43 +0,0 @@ -#!/bin/bash -# -# No header may give a C99 printf call another implementation under its own -# name. htslib.h used to carry "#define snprintf _snprintf" under _WIN32, and -# msvcrt's _snprintf writes no terminating NUL when it truncates, so on Windows -# alone every unchecked snprintf in the engine left its destination NUL-less. -# That reached a user as an abort reading back an 80-byte htsblk.msg the engine -# had filled to capacity. Linux CI cannot see the alias, so this scan is the -# only guard. A bounded wrapper is fine when it has a name of its own, the way -# htssafe.h's sprintfbuff() does. - -set -euo pipefail - -# shellcheck source=tests/testlib.sh -. "${0%"${0##*/}"}./testlib.sh" - -top="${abs_top_srcdir:?not run under make check}" -test -f "$top/src/htslib.h" || fail "no $top/src/htslib.h" - -work=$(mktemp -d "${TMPDIR:-/tmp}/printfalias.XXXXXX") || fail "no tmpdir" -cleanup_push rm -rf "$work" - -alias_re='^[[:space:]]*#[[:space:]]*define[[:space:]]+(v?(f|s|sn)?printf|v?asprintf)[[:space:](]' - -sources() { # sources DIR -> the .c and .h files under it - find "$1" \( -name '*.c' -o -name '*.h' \) -print | LC_ALL=C sort -} - -sources "$top/src" >"$work/files" -test -s "$work/files" || fail "no sources found under $top/src" -xargs grep -nHE "$alias_re" <"$work/files" >"$work/found" || true - -# Control: the scan must fire on the line this test exists to keep out. -mkdir -p "$work/ctl" -printf '#ifdef _WIN32\n#define snprintf _snprintf\n#endif\n' >"$work/ctl/legacy.h" -sources "$work/ctl" | xargs grep -nHE "$alias_re" >"$work/control" || true -test -s "$work/control" || - fail "the scan cannot see a legacy alias, so a clean result proves nothing" - -test ! -s "$work/found" || - fail_dump "a C99 printf call is aliased to another implementation under its own name. Call the real one, or give the wrapper its own name" "$work/found" - -echo "sources scanned: $(lines_of "$work/files"), aliases: 0" diff --git a/tests/491_no-legacy-printf-alias.test b/tests/491_no-legacy-printf-alias.test new file mode 100755 index 000000000..a521a3f2a --- /dev/null +++ b/tests/491_no-legacy-printf-alias.test @@ -0,0 +1,51 @@ +#!/bin/bash +# +# No build may give a C99 printf call another implementation under its own +# name. A Linux build cannot see such an alias, so this scan is the only guard. + +set -euo pipefail + +# shellcheck source=tests/testlib.sh +. "${0%"${0##*/}"}./testlib.sh" + +top="${abs_top_srcdir:?not run under make check}" +test -f "$top/src/htslib.h" || fail "no $top/src/htslib.h" + +work=$(mktemp -d "${TMPDIR:-/tmp}/printfalias.XXXXXX") || fail "no tmpdir" +cleanup_push rm -rf "$work" + +family='v?(f|s|sn)?printf|v?asprintf' +# A macro whose name IS the C99 call, so every later use silently changes +# meaning. A wrapper named otherwise, such as htssafe.h's sprintfbuff, is fine. +source_re="^[[:space:]]*#[[:space:]]*define[[:space:]]+($family)[[:space:](]" +# The same alias handed to the compiler instead: -Dsnprintf=_snprintf, or a +# Visual Studio PreprocessorDefinitions entry. +build_re="($family)=" + +sources() { # sources DIR -> the .c and .h files under it + find "$1" \( -name '*.c' -o -name '*.h' \) -print | LC_ALL=C sort +} + +sources "$top/src" >"$work/files" +# A find that silently narrowed would report a clean tree, so floor the count. +test "$(lines_of "$work/files")" -ge 100 || + fail "only $(lines_of "$work/files") sources found under $top/src" +xargs grep -nHE "$source_re" <"$work/files" >"$work/found" || true + +ls "$top"/configure.ac "$top"/*/Makefile.am "$top"/src/*.vcxproj >"$work/builds" +xargs grep -nHE "$build_re" <"$work/builds" >>"$work/found" || true + +# Control: both scans must fire on the lines this test exists to keep out. +mkdir -p "$work/ctl" +printf '#define snprintf _snprintf\n' >"$work/ctl/legacy.h" +printf 'snprintf=_snprintf\n' \ + >"$work/ctl/legacy.vcxproj" +grep -qE "$source_re" "$work/ctl/legacy.h" || + fail "the source scan cannot see a legacy alias, so a clean result proves nothing" +grep -qE "$build_re" "$work/ctl/legacy.vcxproj" || + fail "the build scan cannot see a legacy alias, so a clean result proves nothing" + +test ! -s "$work/found" || + fail_dump "a C99 printf call is aliased to another implementation under its own name. Call the real one, or give the wrapper its own name" "$work/found" + +echo "scanned $(lines_of "$work/files") sources and $(lines_of "$work/builds") build files, aliases: 0"