Compare commits

...

2 Commits

Author SHA1 Message Date
Xavier Roche
5a043ccf60 Size the new strsafe buffers away from sizeof(char*)
char[8] equals a pointer on LP64, so MSVC's array-vs-pointer heuristic
read the unterminated source as a pointer, skipped the bound and never
aborted; the x64 build failed while Win32 passed. Same trap the existing
comment in that function already warns about.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
2026-07-26 10:24:55 +02:00
Xavier Roche
2193754d9e ARC cache replays a truncated HTTP reason phrase
proxytrack bounded the reason-phrase copy out of an ARC index by
sizeof(pos) - 1 where pos is a const char *, so a stored "404 Not Found"
replays as "404 Not Fou" (and "404 Not" on 32-bit). Use strncatbuff with
the destination's own size.

Fold the nine copies of the buff() family's source-capacity expression
into HTS_SIZEOF_SRC_, applying sizeof to the type so a decayed operand no
longer trips -Wsizeof-array-decay at five call sites; MSVC keeps the old
expression behind the guard HTS_IS_CHAR_BUFFER already uses. The two
other raw strncat calls become strncatbuff, htsbuff_catn stops handing
strnlen the (size_t)-1 sentinel, and htsweb.c no longer compares ep
against a NULL eps.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
2026-07-26 10:13:09 +02:00
8 changed files with 119 additions and 38 deletions

View File

@@ -4583,7 +4583,7 @@ int hts_wait_delayed(htsmoduleStruct * str, lien_adrfilsave *afs,
/* seen as in error */
in_error = back[b].r.statuscode;
in_error_msg[0] = 0;
strncat(in_error_msg, back[b].r.msg, sizeof(in_error_msg) - 1);
strncatbuff(in_error_msg, back[b].r.msg, sizeof(in_error_msg) - 1);
in_error_size = back[b].r.totalsize;
/* don't break, even with "don't take error pages" switch, because we need to process the slot anyway (and cache the error) */
}

View File

@@ -116,6 +116,15 @@ static HTS_UNUSED void abortf_(const char *exp, const char *file, int line) {
#endif
#define HTS_IS_NOT_CHAR_BUFFER(VAR) (!HTS_IS_CHAR_BUFFER(VAR))
/* Source capacity for the buff() family, (size_t)-1 when unknown; sizeof of the
TYPE keeps a decayed operand ("buf + 1") off -Wsizeof-array-decay. */
#if (defined(__GNUC__) && !defined(__cplusplus))
#define HTS_SIZEOF_SRC_(B) \
(HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(__typeof__(B)))
#else
#define HTS_SIZEOF_SRC_(B) (HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B))
#endif
/* Compile-time checks. */
static HTS_UNUSED void htssafe_compile_time_check_(void) {
char array[32];
@@ -205,19 +214,17 @@ static char *strncatbuff_ptr_(char *dest, const char *src, size_t n) {
#if (defined(__GNUC__) && !defined(__cplusplus))
#define strncatbuff(A, B, N) \
__builtin_choose_expr( \
HTS_IS_CHAR_BUFFER(A), \
strncat_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), N, \
"overflow while appending '" #B "' to '" #A "'", __FILE__, \
__LINE__), \
strncatbuff_ptr_((A), (B), (N)))
__builtin_choose_expr(HTS_IS_CHAR_BUFFER(A), \
strncat_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), N, \
"overflow while appending '" #B \
"' to '" #A "'", \
__FILE__, __LINE__), \
strncatbuff_ptr_((A), (B), (N)))
#else
#define strncatbuff(A, B, N) \
(HTS_IS_NOT_CHAR_BUFFER(A) \
? strncat(A, B, N) \
: strncat_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), N, \
: strncat_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), N, \
"overflow while appending '" #B "' to '" #A "'", \
__FILE__, __LINE__))
#endif
@@ -232,9 +239,7 @@ static char *strncatbuff_ptr_(char *dest, const char *src, size_t n) {
#define strcatbuff(A, B) \
__builtin_choose_expr( \
HTS_IS_CHAR_BUFFER(A), \
strncat_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
(size_t) -1, \
strncat_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), (size_t) -1, \
"overflow while appending '" #B "' to '" #A "'", __FILE__, \
__LINE__), \
strcatbuff_ptr_((A), (B)))
@@ -242,9 +247,7 @@ static char *strncatbuff_ptr_(char *dest, const char *src, size_t n) {
#define strcatbuff(A, B) \
(HTS_IS_NOT_CHAR_BUFFER(A) \
? strcat(A, B) \
: strncat_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
(size_t) -1, \
: strncat_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), (size_t) -1, \
"overflow while appending '" #B "' to '" #A "'", \
__FILE__, __LINE__))
#endif
@@ -257,19 +260,17 @@ static char *strncatbuff_ptr_(char *dest, const char *src, size_t n) {
#if (defined(__GNUC__) && !defined(__cplusplus))
#define strcpybuff(A, B) \
__builtin_choose_expr( \
HTS_IS_CHAR_BUFFER(A), \
strcpy_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
"overflow while copying '" #B "' to '" #A "'", __FILE__, \
__LINE__), \
strcpybuff_ptr_((A), (B)))
__builtin_choose_expr(HTS_IS_CHAR_BUFFER(A), \
strcpy_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), \
"overflow while copying '" #B "' to '" #A \
"'", \
__FILE__, __LINE__), \
strcpybuff_ptr_((A), (B)))
#else
#define strcpybuff(A, B) \
(HTS_IS_NOT_CHAR_BUFFER(A) \
? strcpy(A, B) \
: strcpy_safe_(A, sizeof(A), B, \
HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
: strcpy_safe_(A, sizeof(A), B, HTS_SIZEOF_SRC_(B), \
"overflow while copying '" #B "' to '" #A "'", __FILE__, \
__LINE__))
#endif
@@ -286,24 +287,24 @@ static char *strncatbuff_ptr_(char *dest, const char *src, size_t n) {
* Append characters of "B" to "A", "A" having a maximum capacity of "S".
*/
#define strlcatbuff(A, B, S) \
strncat_safe_(A, S, B, HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
(size_t) -1, "overflow while appending '" #B "' to '" #A "'", \
__FILE__, __LINE__)
strncat_safe_(A, S, B, HTS_SIZEOF_SRC_(B), (size_t) -1, \
"overflow while appending '" #B "' to '" #A "'", __FILE__, \
__LINE__)
/**
* Append at most "N" characters of "B" to "A", "A" having a maximum capacity
* of "S".
*/
#define strlncatbuff(A, B, S, N) \
strncat_safe_(A, S, B, HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
N, "overflow while appending '" #B "' to '" #A "'", __FILE__, \
strncat_safe_(A, S, B, HTS_SIZEOF_SRC_(B), N, \
"overflow while appending '" #B "' to '" #A "'", __FILE__, \
__LINE__)
/**
* Copy characters of "B" to "A", "A" having a maximum capacity of "S".
*/
#define strlcpybuff(A, B, S) \
strcpy_safe_(A, S, B, HTS_IS_NOT_CHAR_BUFFER(B) ? (size_t) -1 : sizeof(B), \
strcpy_safe_(A, S, B, HTS_SIZEOF_SRC_(B), \
"overflow while copying '" #B "' to '" #A "'", __FILE__, \
__LINE__)
@@ -422,7 +423,9 @@ static HTS_INLINE HTS_UNUSED htsbuff htsbuff_ptr_(char *buf, size_t cap) {
*/
static HTS_INLINE HTS_UNUSED void htsbuff_catn(htsbuff *b, const char *s,
size_t n) {
const size_t add = strnlen(s, n);
/* the (size_t)-1 "no limit" sentinel would reach strnlen as a bound past
PTRDIFF_MAX */
const size_t add = n != (size_t) -1 ? strnlen(s, n) : strlen(s);
/* Overflow-safe: keep the (potentially huge) 'add' alone on one side. The
maintained invariant len < cap makes 'cap - len' >= 1 (no underflow), so
'add < cap - len' cannot wrap the way 'len + add < cap' could. */

View File

@@ -481,6 +481,28 @@ static int string_safety_selftests(void) {
if (strcmp(buf, "abcd") != 0)
return 1;
/* A decayed source has no known capacity, so the whole tail must land; a
sizeof(char*) capacity would abort here instead. */
{
char src[32] = "0123456789abcdefghij";
char dst[32];
strcpybuff(dst, src + 1);
if (strcmp(dst, "123456789abcdefghij") != 0)
return 1;
}
/* Truncating append: stops at N without aborting, what the status-message
call sites rely on. */
{
char dst[10]; /* never sizeof(char*), or MSVC reads it as a pointer */
dst[0] = '\0';
strncatbuff(dst, "abcdefghijkl", sizeof(dst) - 1);
if (strcmp(dst, "abcdefghi") != 0)
return 1;
}
/* strlcpybuff: explicit-capacity copy into a pointer destination, the form
the migration moves toward */
{
@@ -1299,6 +1321,14 @@ static int st_strsafe(httrackp *opt, int argc, char **argv) {
htsbuff b = htsbuff_array(small);
htsbuff_cat(&b, src);
} else if (strcmp(argv[0], "overflow-src") == 0) {
/* Array source with no NUL: its capacity still comes from sizeof(), so
the bounded strlen aborts rather than running off the array. */
char nonul[6]; /* never sizeof(char*), per the note above */
char big[64];
memset(nonul, src[0], sizeof(nonul));
strcpybuff(big, nonul);
} else {
strcpybuff(small, src);
}
@@ -4807,8 +4837,8 @@ static const struct selftest_entry {
{"unescape-bounds", "", "unescapers reserve the NUL byte (no 1-byte OOB)",
st_unescape_bounds},
{"hashtable", "<count|file>", "coucal hashtable stress test", st_hashtable},
{"strsafe", "[overflow|overflow-buff [str]]", "bounded string-op self-test",
st_strsafe},
{"strsafe", "[overflow|overflow-buff|overflow-src [str]]",
"bounded string-op self-test", st_strsafe},
{"copyopt", "", "copy_htsopt option-copy self-test", st_copyopt},
{"pause", "", "randomized inter-file pause target self-test", st_pause},
{"relative", "<link> <curr-file>", "relative link between two paths",

View File

@@ -671,10 +671,10 @@ int __cdecl htsshow_loop(t_hts_callbackarg * carg, httrackp * opt, lien_back * b
char *eps = strchr(back[i].url_adr, '/');
int count;
if (ep != NULL && ep < eps
&& (count = (int) (ep - back[i].url_adr)) < 4) {
if (ep != NULL && eps != NULL && ep < eps &&
(count = (int) (ep - back[i].url_adr)) < 4) {
proto[0] = '\0';
strncat(proto, back[i].url_adr, count);
strncatbuff(proto, back[i].url_adr, count);
}
}
snprintf(StatsBuffer[index].state, sizeof(StatsBuffer[index].state),

View File

@@ -2191,7 +2191,7 @@ static PT_Element PT_ReadCache__Arc_u(PT_Index index_, const char *url,
}
if ((pos = getArcField(index->line, 2)) != NULL) {
r->msg[0] = '\0';
strncat(r->msg, pos, sizeof(pos) - 1);
strncatbuff(r->msg, pos, sizeof(r->msg) - 1);
}
while(linput(index->file, index->line, sizeof(index->line) - 1)
&& index->line[0] != '\0') {

View File

@@ -47,3 +47,18 @@ case "$err" in
exit 1
;;
esac
# An array source with no NUL must abort on the source bound rather than run
# off the array: the array half of the source-capacity selection.
err=$(httrack -#test=strsafe overflow-src "x" 2>&1) || true
case "$err" in
*"strsafe: NOT aborted"*)
echo "unterminated source array was NOT caught" >&2
exit 1
;;
*"size < sizeof_source"*) ;;
*)
echo "expected htssafe source-bound abort, got: $err" >&2
exit 1
;;
esac

View File

@@ -0,0 +1,32 @@
#!/bin/bash
# An ARC entry's HTTP reason phrase must survive a proxytrack --convert
# round-trip; it used to be clipped to sizeof(char*) - 1 bytes.
set -euo pipefail
dir=$(mktemp -d)
trap 'rm -rf "$dir"' EXIT
printf 'HTTP/1.1 404 Not Found Here At All\r\nContent-Type: text/html\r\nLast-Modified: Wed, 01 Jan 2025 00:00:00 GMT\r\nContent-Length: 5\r\n\r\n' >"$dir/hdr"
printf 'hello' >"$dir/body"
alen=$(($(wc -c <"$dir/hdr") + $(wc -c <"$dir/body")))
# ARC 1.0: filedesc record and version block, then per entry
# <nl> <URL-record> <nl> <headers> <body>; the record's last field is that length.
{
printf 'filedesc://t.arc 0.0.0.0 20250101000000 text/plain 200 - - 0 t.arc 9\n'
printf '2 0 test\n'
printf '\n\n'
printf 'http://example.com/page.html 0.0.0.0 20250101000000 text/html 404 - - 0 t.arc %d\n' "$alen"
cat "$dir/hdr" "$dir/body"
} >"$dir/in.arc"
proxytrack --convert "$dir/out.arc" "$dir/in.arc" >/dev/null 2>&1
# HTTP/1.0 is the writer's own prefix (the input says 1.1), so a verbatim copy
# of the input cannot satisfy this match.
grep -aqF 'HTTP/1.0 404 Not Found Here At All' "$dir/out.arc" || {
echo "reason phrase lost in the ARC round-trip:" >&2
grep -a '^HTTP/' "$dir/out.arc" >&2 || echo "(no status line at all)" >&2
exit 1
}

View File

@@ -147,6 +147,7 @@ TESTS = \
50_local-contentcodings.test \
51_local-update-codec.test \
52_local-socks5.test \
53_local-proxytrack-arc-reason.test \
53_local-proxytrack-cache-corrupt.test \
54_local-update-truncate-purge.test \
55_local-chunked.test \