PR #24817 opened by Jake (jakefineman)
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24817
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24817.patch
**Follow-up, not depending on any open PR.** This is one commit on unmodified
upstream master (ff2059afd9, "tests/fate/dashenc: add a test for a large init
segment"), branch `dnn-proc-sws-dst-array`. It fixes a stack-buffer-overflow
found while verifying evidence for an unrelated follow-up
(`dnn-probe-task-zeroinit`, now #24800); it reproduces identically on that
commit and on plain unmodified master, so it is not a regression from that or
any other pending series. The evidence below was produced on commit c0ed9f25fc;
the commit in this PR, b908f06b70, is the same change re-signed, with an
identical tree (ea8b39a8b9).
**Problem.** `libavfilter/dnn/dnn_io_proc.c` has two structurally identical
functions that convert between an `AVFrame` and a `DNNData` tensor,
`ff_proc_from_dnn_to_frame()` (tensor -> frame, used for a model's output) and
`ff_proc_from_frame_to_dnn()` (frame -> tensor, used for a model's input). Both
declare a single `void *middle_data` local to hold a temporary NCHW-layout
buffer, and both, on the `DL_NCHW` branch, take that one variable's address and
hand it to `sws_scale()` as the destination or source plane array:
void *middle_data = NULL;
...
if (output->layout == DL_NCHW) {
middle_data = av_malloc(plane_size * output->dims[1]);
...
dst_data = &middle_data; // dnn_io_proc.c:79
linesize[0] = frame->width * 3;
}
...
sws_scale(sws_ctx, ..., (uint8_t * const*)dst_data, linesize);
(and the mirror image at line 218, `src_data = &middle_data;`, in
`ff_proc_from_frame_to_dnn()`.)
`sws_scale()`'s `dst`/`src` parameters are a 4-element array, `uint8_t *const
dst[4]` / `src[4]`; its internal dispatch reads `dst[1]`, `dst[2]`, `dst[3]`
(or the `src` equivalents) to decide how many planes it is dealing with,
regardless of how many planes this particular conversion actually uses.
`&middle_data` is the address of one 8-byte stack slot, not a 4-element array,
so those reads walk 8/16/24 bytes past it into whatever the compiler placed
next on the stack (in practice the `planar_data[4]` array declared right after
it). This is a genuine out-of-bounds stack read on every `dnn_processing` call
whose model uses `DL_NCHW` output or input layout together with an RGB24 or
BGR24 frame -- which is the normal case for ONNX, OpenVINO, and NCHW-layout
TensorFlow models. It doesn't depend on the model's numerical content, a
malformed model, or fault injection.
**Fix.** Give `dst_data`/`src_data` real 4-element backing storage instead of
aliasing the single pointer, with the allocated buffer in slot 0 and the rest
zeroed:
- dst_data = &middle_data;
+ dst_data_buf[0] = middle_data;
+ dst_data = dst_data_buf;
and the same shape of change for `src_data`/`src_data_buf`. `sws_scale()` still
only ever writes/reads slot 0 for these packed-destination conversions; the fix
just makes the array it's handed actually have 4 real elements to read past
slot 0 into, instead of running off the end of a single stack variable. The
`av_free(middle_data)` cleanup at the end of both functions is unchanged --
`dst_data_buf[0]`/`src_data_buf[0]` hold a copy of the same pointer, not a
second allocation, so there is no double free and no new leak.
I checked every other `sws_scale()` call in this file and in
`vf_dnn_processing.c` for the same aliasing shape (a scalar pointer's address
cast to a plane array). The `bbox_data` call in `dnn_io_proc.c`'s bounding-box
crop path takes `&bbox_data` where `bbox_data` is declared `uint8_t
*bbox_data[4]` -- a real 4-element array, so `&bbox_data` and `bbox_data` are
the same address with different pointer-to-array vs pointer-to-first-element
types; that one is not an instance of this bug. Every remaining `sws_scale()`
call in both files passes either `frame->data` (AVFrame's own multi-element
array) or a locally declared 4-element array. `dst_data`/`src_data` are the
only two aliased single-pointer instances in the codebase searched.
**Reproducer.**
ffmpeg -threads 1 -i puppies.ogv -an \
-vf
"fps=2,format=rgb24,dnn_processing=dnn_backend=openvino:model=proc_valid_720.onnx:input=input:output=output"
\
-f framemd5 out.md5
built with `--toolchain=clang-asan --enable-libopenvino`, aborts under ASan on
the very first frame with a stack-buffer-overflow read inside
`ff_proc_from_dnn_to_frame`, offset 72 into the frame (the byte immediately
after `middle_data`'s 8-byte slot, running into where `planar_data` begins).
With this commit applied, the same command is clean of that report.
[ATTACH:results/before-ov-asan.log] [ATTACH:results/after-ov-asan.log]
**Testing.** Darwin 27.0.0 arm64 (Apple Silicon). OpenVINO 2026.4.0, ONNX
Runtime 1.30.0, and TensorFlow (C API) all enabled in one configure. Two source
trees built four ways each (ASan and release): "before" = unmodified master
ff2059afd9, "after" = this commit on the same base. All of the below was
independently re-run and re-checked by a second pass after the original
evidence run, using the same binaries and pinned model/clip hashes; every
number matched. [ATTACH:MODELS.txt]
*ASan, both aliasing instances independently confirmed live, not just
structurally identical:*
- `ff_proc_from_dnn_to_frame()` (the `dst_data` instance, line 79): the
reproducer above, using the synthetic identity model `proc_valid_720.onnx` via
`dnn_backend=openvino`. Aborts on "before" with the stack-buffer-overflow shown
above, from the OpenVINO async inference callback thread. Clean on "after".
[ATTACH:results/before-ov-asan.log] [ATTACH:results/after-ov-asan.log]
Independently re-run: [ATTACH:results/verify-before-ov-asan.log]
[ATTACH:results/verify-after-ov-asan.log]
- `ff_proc_from_frame_to_dnn()` (the `src_data` instance, line 218): the same
`dnn_processing` filter, this time with two real NCHW ONNX Model Zoo
fast-neural-style models via `dnn_backend=onnx` -- `candy-9.onnx` and
`mosaic-9.onnx` (sha256s in the attached model manifest), both fixed-shape
`1x3x224x224` NCHW in/out, run at `scale=224:224,format=rgb24`. Both abort on
"before" with the overflow located directly inside
`ff_proc_from_frame_to_dnn`'s `middle_data` frame at offset 72 -- confirming
this sibling instance is independently exploitable, not merely a structural
lookalike of the first. Both clean on "after".
[ATTACH:results/before-candy-asan.log] [ATTACH:results/after-candy-asan.log]
[ATTACH:results/before-mosaic-asan.log] [ATTACH:results/after-mosaic-asan.log]
- BGR24 (only RGB24 was covered by the runs above): `candy-9.onnx` at
`scale=224:224,format=bgr24`, five frames. Aborts on "before" with the same
`ff_proc_from_frame_to_dnn` stack-buffer-overflow. Clean on "after" (only the
unrelated at-exit SEGV below remains, and the five frames of framemd5 output
are written successfully before it).
[ATTACH:results/verify-before-candy-bgr24-asan.log]
[ATTACH:results/verify-after-candy-bgr24-asan.log]
An unrelated, pre-existing OpenVINO/TBB at-exit SEGV under ASan
(`tbb::detail::r1::__TBB_InitOnce::~__TBB_InitOnce()`, on process `exit()`)
remains on this machine/toolchain/OpenVINO version in every "after" ASan run
above; it reproduces identically on plain `ffmpeg -version` with no DNN filter
at all, on both the before and after trees, so it is independent of this bug
and not addressed by this commit. [ATTACH:results/before-version-control.log]
[ATTACH:results/after-version-control.log]
*Release build, whole clip (puppies.ogv, 589 frames, 1280x720 at 2fps
sampling), framemd5 output byte-for-byte identical before/after:*
- the OpenVINO/`proc_valid_720.onnx` NCHW path above
[ATTACH:results/before-ov-rel-whole.md5] [ATTACH:results/after-ov-rel-whole.md5]
- the ONNX/`candy-9.onnx` path [ATTACH:results/before-candy-rel-whole.md5]
[ATTACH:results/after-candy-rel-whole.md5], independently re-run:
[ATTACH:results/verify-before-candy-rel-whole.md5]
[ATTACH:results/verify-after-candy-rel-whole.md5]
- the ONNX/`mosaic-9.onnx` path [ATTACH:results/before-mosaic-rel-whole.md5]
[ATTACH:results/after-mosaic-rel-whole.md5]
- the BGR24/`candy-9.onnx` path (gap closed)
[ATTACH:results/verify-before-candy-bgr24-rel-whole.md5]
[ATTACH:results/verify-after-candy-bgr24-rel-whole.md5]
This is the expected outcome: the aliasing read pulls garbage bytes off the
stack that `sws_scale()` never writes anywhere for these packed-destination
conversions, so the fix changes what ASan's redzone sees but not the actual
visible output.
*The two real models actually transform the frame, so "identical before/after"
isn't a degenerate no-op check*: the after-fix `candy-9.onnx` and
`mosaic-9.onnx` whole-clip framemd5 output each differs from a no-filter
baseline run over the same clip at the same scale/format
(`scale=224:224,format=rgb24`, no `dnn_processing`), and the after-fix BGR24
`candy-9.onnx` output differs from the equivalent BGR24 no-filter baseline.
[ATTACH:results/verify-baseline-nofilter-rgb24.md5]
[ATTACH:results/verify-baseline-nofilter-bgr24.md5]
*Unchanged paths, also byte-for-byte identical before/after (these never
allocate `middle_data` or touch the aliased pointer):*
- NHWC TensorFlow: `tf_rgb_valid.pb` via `dnn_backend=tensorflow` into rgb24
(its output layout is NHWC/packed, so it never takes the `DL_NCHW` branch)
[ATTACH:results/before-tf-rel-whole.md5] [ATTACH:results/after-tf-rel-whole.md5]
- planar/gray output: `tf_y_valid.pb` via `dnn_backend=tensorflow` into
`format=gray`, which uses `frame->data` directly in the `AV_PIX_FMT_GRAY8`
switch case, never the RGB24/BGR24 case that allocates `middle_data`
[ATTACH:results/before-gray-rel-whole.md5]
[ATTACH:results/after-gray-rel-whole.md5]
*Second clip (supplementary, not required to reproduce the numbers above)*:
`mosaic-9.onnx` against a second, unrelated 1280x534 clip (Blender Foundation's
"Tears of Steel," sha256 in the attached model manifest) reproduces the same
pattern -- stack-buffer-overflow on "before," clean on "after," and the first
100 frames of release framemd5 output are byte-for-byte identical before/after.
[ATTACH:results/verify-before-mosaic-tos-asan.log]
[ATTACH:results/verify-after-mosaic-tos-asan.log]
[ATTACH:results/verify-before-mosaic-tos-rel.md5]
[ATTACH:results/verify-after-mosaic-tos-rel.md5]
`make fate-source` passes on the "after" tree. This tree has no `fate-dnn*`
tests to run. [ATTACH:results/fate-source-after.log]
Evidence kit (build script, evidence-generation script, model/clip manifest
with pinned sha256s, and raw logs for every result above, including the
independent re-verification pass) is attached: [ATTACH:build.sh],
[ATTACH:run-evidence.sh], [ATTACH:gen-identity-models.py],
[ATTACH:gen-identity-tf-model.py] and [ATTACH:MODELS.txt]. `run-evidence.sh`
regenerates every number in this description from a clean checkout given the
pinned model sha256s and the test clip.
>From b908f06b70f62c4c3295a3bdf9d4f6c5330ca0b7 Mon Sep 17 00:00:00 2001
From: Jake Fineman <[email protected]>
Date: Wed, 30 Sep 2026 10:08:02 -0400
Subject: [PATCH] avfilter/dnn_io_proc: fix stack overflow in NCHW<->frame
sws_scale calls
ff_proc_from_dnn_to_frame() and ff_proc_from_frame_to_dnn() each declare a
single `void *middle_data` local and, on the DL_NCHW layout branch, take its
address (`dst_data = &middle_data;` / `src_data = &middle_data;`) and pass
that as sws_scale()'s dst/src plane-array argument after casting to
`uint8_t *const *`. sws_scale()'s dst/src parameters are a 4-element array
(uint8_t *const dst[4]/src[4]); its internal dispatch reads dst[1..3] or
src[1..3] regardless of how many planes the actual conversion uses. Since
`&middle_data` only ever points at one 8-byte stack slot, those reads walk
past it into whatever is next on the stack: an out-of-bounds stack read on
every dnn_processing call whose model uses NCHW layout output or input into
an RGB24/BGR24 frame.
Fix both by giving dst_data/src_data real 4-element backing storage
(dst_data_buf/src_data_buf, zero-initialized) with the allocated buffer in
slot 0, instead of aliasing the single pointer.
Tested on arm64 macOS 27.0 (M1 Ultra), clang-asan toolchain, with
--enable-libopenvino (2026.4.0), --enable-libonnxruntime (1.30.0) and
--enable-libtensorflow, against unmodified upstream master (ff2059afd9) and
against this commit applied on top of the same base:
- Reproduced the stack-buffer-overflow on unmodified master two ways:
- ff_proc_from_dnn_to_frame(): `ffmpeg -threads 1 -i puppies.ogv -an -vf
"fps=2,format=rgb24,dnn_processing=dnn_backend=openvino:
model=proc_valid_720.onnx:input=input:output=output" -f framemd5 out.md5`
aborts under ASan on the first frame (offset-72 read into the
`middle_data`/`planar_data` stack slots, from the OpenVINO async
inference callback thread).
- ff_proc_from_frame_to_dnn(): the same dnn_processing filter with two real
ONNX Model Zoo fast-neural-style models (candy-9.onnx,
sha256 9d11a3529d1e547da6ae07201d93484dbab2ec0a3614535752c8f40f0fe2968a;
mosaic-9.onnx,
sha256 fa646dedade881243f8d5a2ceb7de2b93675b21fc24f7482894ac4851a9a0a47;
both NCHW 1x3x224x224 in/out) via dnn_backend=onnx aborts under ASan the
same way, this time with the overflow located directly in
ff_proc_from_frame_to_dnn's `middle_data` frame (offset 72), confirming
the sibling instance flagged at review is independently live, not just
structurally identical.
With this commit applied, all three of the above runs are clean of the
stack-buffer-overflow report (an unrelated, pre-existing OpenVINO/TBB
at-exit SEGV under ASan remains on this machine/toolchain/OpenVINO version;
it reproduces identically before and after this commit, and even on plain
`ffmpeg -version` with no DNN filter at all, so it is unrelated to this fix
and not addressed here).
- Whole-clip (puppies.ogv, 589 frames) framemd5 output from a release
(non-ASan) build is byte-for-byte identical before/after this commit for:
the NCHW OpenVINO identity model (proc_valid_720.onnx) into rgb24, and for
both candy-9.onnx and mosaic-9.onnx into rgb24 via dnn_backend=onnx. This
is expected: the aliasing read pulls garbage bytes that sws_scale never
writes to a packed destination/never reads for these plane counts, so the
visible output is unaffected; only ASan's redzone instrumentation makes the
read observable.
- Unchanged paths are also byte-for-byte identical before/after: the NHWC
TensorFlow path (tf_rgb_valid.pb into rgb24, which never takes the
DL_NCHW/middle_data branch) and the planar/gray output path (tf_y_valid.pb
into gray8, which uses frame->data directly and never allocates
middle_data).
- `make fate-source` passes with this commit. This tree has no fate-dnn*
tests to run.
Signed-off-by: yakimoto <[email protected]>
---
libavfilter/dnn/dnn_io_proc.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavfilter/dnn/dnn_io_proc.c b/libavfilter/dnn/dnn_io_proc.c
index 0a18c56b84..ec4d012319 100644
--- a/libavfilter/dnn/dnn_io_proc.c
+++ b/libavfilter/dnn/dnn_io_proc.c
@@ -46,6 +46,7 @@ int ff_proc_from_dnn_to_frame(AVFrame *frame, DNNData
*output, void *log_ctx)
int linesize[4] = { 0 };
void **dst_data = NULL;
void *middle_data = NULL;
+ void *dst_data_buf[4] = { NULL, NULL, NULL, NULL };
uint8_t *planar_data[4] = { 0 };
int plane_size = frame->width * frame->height * sizeof(uint8_t);
enum AVPixelFormat src_fmt = AV_PIX_FMT_NONE;
@@ -76,7 +77,8 @@ int ff_proc_from_dnn_to_frame(AVFrame *frame, DNNData
*output, void *log_ctx)
ret = AVERROR(ENOMEM);
goto err;
}
- dst_data = &middle_data;
+ dst_data_buf[0] = middle_data;
+ dst_data = dst_data_buf;
linesize[0] = frame->width * 3;
}
@@ -186,6 +188,7 @@ int ff_proc_from_frame_to_dnn(AVFrame *frame, DNNData
*input, void *log_ctx)
int linesize[4] = { 0 };
void **src_data = NULL;
void *middle_data = NULL;
+ void *src_data_buf[4] = { NULL, NULL, NULL, NULL };
uint8_t *planar_data[4] = { 0 };
int plane_size = frame->width * frame->height * sizeof(uint8_t);
enum AVPixelFormat dst_fmt = AV_PIX_FMT_NONE;
@@ -215,7 +218,8 @@ int ff_proc_from_frame_to_dnn(AVFrame *frame, DNNData
*input, void *log_ctx)
ret = AVERROR(ENOMEM);
goto err;
}
- src_data = &middle_data;
+ src_data_buf[0] = middle_data;
+ src_data = src_data_buf;
linesize[0] = frame->width * 3;
}
--
2.52.0
_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
