From 297452c33fa85de4f620239828f98a2945728881 Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Wed, 12 Aug 2026 13:20:16 -0700 Subject: [PATCH 1/2] fix(file_buffer): stop unsigned underflow turning bounds checks into passes buffered_seek() handled a negative offset with ctx->filebuffer_pos += offset; if (ctx->filebuffer_pos < 0) but filebuffer_pos is unsigned int, so the addition wraps instead of going negative and the check can never be true. The guard below it compares filebuffer_pos + startbytes_pos against 0 and is dead for the same reason, so the "seek before buffer start" fatal() is unreachable. Seeking back further than the current position therefore left filebuffer_pos at roughly 2^32. buffered_read() then evaluated bytes <= ctx->bytesinbuffer - ctx->filebuffer_pos which underflows too, so the bounds check passed and memcpy() read from filebuffer + 2.3e9 against a 1 MB buffer. Observed on a malformed WTV file, where a corrupt chunk offset produces buffered_seek(ctx, -1969339576) with filebuffer_pos = 1048576: filebuffer_pos -> 2326676296 bytesinbuffer = 1048576 bytesinbuffer - filebuffer_pos = 1969339576 (underflow) memcpy(dst, filebuffer + 2326676296, 32) -> SIGSEGV Do the seek arithmetic in int64_t so both guards work as written, and route the three buffer-space checks through a buffered_bytes_left() helper that returns 0 when the position is past the end. buffered_read_byte() had the same hazard and would have indexed filebuffer[] out of bounds. Input files are untrusted, so this is a bounds-check fix, not only a crash fix. --- src/lib_ccx/file_buffer.h | 20 +++++++++++++++++--- src/lib_ccx/file_functions.c | 16 ++++++++++++---- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/lib_ccx/file_buffer.h b/src/lib_ccx/file_buffer.h index 11973f5f1..a5e763df6 100644 --- a/src/lib_ccx/file_buffer.h +++ b/src/lib_ccx/file_buffer.h @@ -17,6 +17,20 @@ */ size_t buffered_read_opt(struct ccx_demuxer *ctx, unsigned char *buffer, size_t bytes); +/** + * Bytes still unread in the file buffer. + * + * bytesinbuffer and filebuffer_pos are both unsigned, so subtracting them without + * checking their order wraps around and turns a bounds check into a pass. Callers use + * this instead of doing the subtraction themselves. + */ +static inline size_t buffered_bytes_left(struct ccx_demuxer *ctx) +{ + if (ctx->filebuffer_pos >= ctx->bytesinbuffer) + return 0; + return (size_t)(ctx->bytesinbuffer - ctx->filebuffer_pos); +} + /** * Skip bytes from file buffer and if needed also seek file for number of bytes. * @@ -24,7 +38,7 @@ size_t buffered_read_opt(struct ccx_demuxer *ctx, unsigned char *buffer, size_t static size_t inline buffered_skip(struct ccx_demuxer *ctx, unsigned int bytes) { size_t result; - if (bytes <= ctx->bytesinbuffer - ctx->filebuffer_pos) + if (bytes <= buffered_bytes_left(ctx)) { ctx->filebuffer_pos += bytes; result = bytes; @@ -43,7 +57,7 @@ static size_t inline buffered_skip(struct ccx_demuxer *ctx, unsigned int bytes) static size_t inline buffered_read(struct ccx_demuxer *ctx, unsigned char *buffer, size_t bytes) { size_t result; - if (bytes <= ctx->bytesinbuffer - ctx->filebuffer_pos) + if (bytes <= buffered_bytes_left(ctx)) { if (buffer != NULL) memcpy(buffer, ctx->filebuffer + ctx->filebuffer_pos, bytes); @@ -70,7 +84,7 @@ static size_t inline buffered_read(struct ccx_demuxer *ctx, unsigned char *buffe static size_t inline buffered_read_byte(struct ccx_demuxer *ctx, unsigned char *buffer) { size_t result = 0; - if (ctx->bytesinbuffer - ctx->filebuffer_pos) + if (buffered_bytes_left(ctx)) { if (buffer) { diff --git a/src/lib_ccx/file_functions.c b/src/lib_ccx/file_functions.c index fb8653b9f..194d4b946 100644 --- a/src/lib_ccx/file_functions.c +++ b/src/lib_ccx/file_functions.c @@ -230,17 +230,25 @@ void buffered_seek(struct ccx_demuxer *ctx, int offset) position_sanity_check(ctx); if (offset < 0) { - ctx->filebuffer_pos += offset; - if (ctx->filebuffer_pos < 0) + /* filebuffer_pos and startbytes_pos are unsigned, so "filebuffer_pos += offset" + wrapped around instead of going negative and neither guard below could ever + fire. A backward seek past the start of the buffer left filebuffer_pos at + roughly 2^32, which buffered_read() then trusted. Do the arithmetic signed. */ + int64_t newpos = (int64_t)ctx->filebuffer_pos + offset; + if (newpos < 0) { // We got into the start buffer (hopefully) - if ((ctx->filebuffer_pos + ctx->startbytes_pos) < 0) + if ((int64_t)ctx->startbytes_pos + newpos < 0) { fatal(CCX_COMMON_EXIT_BUG_BUG, "PANIC: Attempt to seek before buffer start, this is a bug!"); } - ctx->startbytes_pos += ctx->filebuffer_pos; + ctx->startbytes_pos = (unsigned int)((int64_t)ctx->startbytes_pos + newpos); ctx->filebuffer_pos = 0; } + else + { + ctx->filebuffer_pos = (unsigned int)newpos; + } } else { From 5e2f301ecdc79c0b05b2c3a791495b2d9305b052 Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Thu, 13 Aug 2026 00:18:33 -0700 Subject: [PATCH 2/2] fix(file_buffer): make the buffer-position narrowing explicit MSVC warns C4267 on "filebuffer_pos += bytes": filebuffer_pos is an unsigned int and bytes is a size_t. The warning predates this branch -- it fires 54 times on master -- but the guard this commit series added is what makes the conversion provably safe, so state that in the code instead of leaving a warning that a reader has to re-derive. bytes is known to be no larger than what is left of the buffer, and that remainder is itself an unsigned int, so the value always fits. --- src/lib_ccx/file_buffer.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/lib_ccx/file_buffer.h b/src/lib_ccx/file_buffer.h index a5e763df6..577151c80 100644 --- a/src/lib_ccx/file_buffer.h +++ b/src/lib_ccx/file_buffer.h @@ -61,7 +61,10 @@ static size_t inline buffered_read(struct ccx_demuxer *ctx, unsigned char *buffe { if (buffer != NULL) memcpy(buffer, ctx->filebuffer + ctx->filebuffer_pos, bytes); - ctx->filebuffer_pos += bytes; + /* The guard above proved bytes fits in what is left of the buffer, and that + remainder is an unsigned int, so narrowing here cannot lose data. Say so + explicitly: MSVC warns C4267 on the implicit size_t -> unsigned int. */ + ctx->filebuffer_pos += (unsigned int)bytes; result = bytes; } else