From ee366cdf78dd4f9f16df8d0c9bee8a7015119d3d Mon Sep 17 00:00:00 2001 From: SteveGilvarry Date: Thu, 11 Jun 2026 19:42:52 +1000 Subject: [PATCH] fix: rotate/flip segfault from AV_CEIL_RSHIFT on unsigned dimensions Image::Rotate and Image::Flip computed chroma plane dimensions with AV_CEIL_RSHIFT(width, log2_chroma_w). The macro's runtime form is -((-(a)) >> (b)), which relies on arithmetic right shift of a negative value and is only valid for signed operands - FFmpeg always passes int. Image::width/height are unsigned, so the negation wraps, the shift is logical, and the result is 2^31 + ceil(w/2^b) instead of ceil(w/2^b): for 1280x720 the chroma rotate received src_w=2147484288 and src_h=2147484008 (captured in gdb), writing gigabytes out of bounds. Effect: zmc's decoder thread segfaulted on the first decoded frame of any monitor with a rotated or flipped orientation and a planar pixel format - a monitor with decoding Always crash-loops. Replace the macro at both sites with an explicit unsigned ceiling shift helper and a comment documenting the trap. Tests: new tests/zm_image.cpp covers Rotate 90/180/270 and Flip on YUV420P with per-plane marker pixels, plus odd dimensions exercising the chroma ceiling. The Rotate cases segfault before this fix (verified, exit 139) and pass after. Full suite 85/85 via ctest. Live-verified on a 1280x720 ROTATE_270 monitor with decoding Always: pre-fix crash within seconds, post-fix 90s clean run under gdb. Co-Authored-By: Claude Fable 5 --- src/zm_image.cpp | 21 ++++-- tests/CMakeLists.txt | 1 + tests/zm_image.cpp | 162 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 6 deletions(-) create mode 100644 tests/zm_image.cpp diff --git a/src/zm_image.cpp b/src/zm_image.cpp index 095fc2ae9..3a5a7218f 100644 --- a/src/zm_image.cpp +++ b/src/zm_image.cpp @@ -3121,6 +3121,15 @@ void Image::Fill(Rgb colour, int density, const Polygon &polygon) { } namespace { +// Ceiling-divide an unsigned dimension by 2^shift. AV_CEIL_RSHIFT must NOT +// be used here: its runtime form is -((-(a)) >> (b)), which relies on +// arithmetic shift of a negative value and silently produces 2^31 + a/2^b +// when `a` is unsigned (logical shift), sending plane loops billions of +// samples out of bounds. +inline unsigned int ceil_rshift(unsigned int a, unsigned int shift) { + return (a + (1u << shift) - 1) >> shift; +} + // Rotate `src_w` × `src_h` plane (bpp bytes per sample, src_linesize stride) // into dst (dst_linesize stride) according to angle (90/180/270). // For 90/270: dst dims are src_h × src_w. For 180: dst dims are src_w × src_h. @@ -3224,10 +3233,10 @@ void Image::Rotate(int angle) { if (planar) { // Each plane has 1 byte per sample. Plane 0 is luma at full resolution; // planes 1/2 are chroma subsampled by desc->log2_chroma_w/h. Use ceiling - // (AV_CEIL_RSHIFT) — flooring would drop the last chroma column/row for - // odd luma dimensions and leave part of U/V unrotated. - const unsigned int cw = AV_CEIL_RSHIFT(width, desc->log2_chroma_w); - const unsigned int ch = AV_CEIL_RSHIFT(height, desc->log2_chroma_h); + // division — flooring would drop the last chroma column/row for odd luma + // dimensions and leave part of U/V unrotated. + const unsigned int cw = ceil_rshift(width, desc->log2_chroma_w); + const unsigned int ch = ceil_rshift(height, desc->log2_chroma_h); rotate_plane(src_planes[0], src_strides[0], width, height, dst_planes[0], dst_strides[0], 1, angle); rotate_plane(src_planes[1], src_strides[1], cw, ch, @@ -3306,8 +3315,8 @@ void Image::Flip( bool leftright ) { if (planar) { // Ceiling division so odd luma dimensions don't drop the last chroma // column/row. - const unsigned int cw = AV_CEIL_RSHIFT(width, desc->log2_chroma_w); - const unsigned int ch = AV_CEIL_RSHIFT(height, desc->log2_chroma_h); + const unsigned int cw = ceil_rshift(width, desc->log2_chroma_w); + const unsigned int ch = ceil_rshift(height, desc->log2_chroma_h); flip_plane(src_planes[0], src_strides[0], width, height, dst_planes[0], dst_strides[0], 1, leftright); flip_plane(src_planes[1], src_strides[1], cw, ch, diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c13a6f3d4..600ded72a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -17,6 +17,7 @@ set(TEST_SOURCES zm_comms.cpp zm_crypt.cpp zm_font.cpp + zm_image.cpp zm_onvif_renewal.cpp zm_pixformat.cpp zm_poly.cpp diff --git a/tests/zm_image.cpp b/tests/zm_image.cpp new file mode 100644 index 000000000..3251628dd --- /dev/null +++ b/tests/zm_image.cpp @@ -0,0 +1,162 @@ +/* + * This file is part of the ZoneMinder Project. See AUTHORS file for Copyright information + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see . + */ + +#include "zm_catch2.h" + +#include "zm_config.h" +#include "zm_image.h" +#include "zm_rgb.h" + +extern "C" { +#include +} + +#include + +namespace { + +// Image::Initialise() loads the timestamp font from the DB-backed config, +// which the test binary doesn't have; point it at the test fixture font. +void bootstrap_image_config() { + config.font_file_location = "data/fonts/04_valid.zmfnt"; +} + +struct Planes { + uint8_t *data[4] = {nullptr, nullptr, nullptr, nullptr}; + int stride[4] = {0, 0, 0, 0}; +}; + +// View an Image's buffer with the same layout Image uses internally +// (av_image_fill_arrays, align 32). +Planes plane_view(Image &image, AVPixelFormat fmt, int w, int h) { + Planes planes; + REQUIRE(av_image_fill_arrays(planes.data, planes.stride, image.Buffer(), + fmt, w, h, 32) > 0); + return planes; +} + +} // namespace + +// Regression: Image::Rotate/Flip computed chroma plane dimensions with +// AV_CEIL_RSHIFT on unsigned operands. The macro's runtime form +// -((-(a)) >> (b)) is only valid for signed types; with unsigned width it +// yields 2^31 + width/2, sending the chroma loops billions of samples out +// of bounds (segfault on every rotated planar image - decoder thread crash +// on any monitor with a rotated orientation). +TEST_CASE("Image::Rotate YUV420P", "[image]") { + bootstrap_image_config(); + const int w = 1280, h = 720; + Image image(w, h, ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUV420P); + REQUIRE(image.Buffer() != nullptr); + + // Distinct fill per plane + a marker pixel in each + Planes src = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + memset(src.data[0], 0x10, static_cast(src.stride[0]) * h); + memset(src.data[1], 0x20, static_cast(src.stride[1]) * (h / 2)); + memset(src.data[2], 0x30, static_cast(src.stride[2]) * (h / 2)); + src.data[0][20 * src.stride[0] + 10] = 200; // luma (10, 20) + src.data[1][30 * src.stride[1] + 40] = 210; // U (40, 30) + src.data[2][50 * src.stride[2] + 60] = 220; // V (60, 50) + + SECTION("rotate 90") { + image.Rotate(90); + REQUIRE(image.Width() == static_cast(h)); + REQUIRE(image.Height() == static_cast(w)); + + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, h, w); + // 90deg: (sx, sy) -> (dx, dy) = (src_h-1-sy, sx) + REQUIRE(dst.data[0][10 * dst.stride[0] + (h - 1 - 20)] == 200); + // chroma plane is (w/2 x h/2): (40, 30) -> (h/2-1-30, 40) + REQUIRE(dst.data[1][40 * dst.stride[1] + (h / 2 - 1 - 30)] == 210); + REQUIRE(dst.data[2][60 * dst.stride[2] + (h / 2 - 1 - 50)] == 220); + } + + SECTION("rotate 180") { + image.Rotate(180); + REQUIRE(image.Width() == static_cast(w)); + REQUIRE(image.Height() == static_cast(h)); + + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + // 180deg: (sx, sy) -> (src_w-1-sx, src_h-1-sy) + REQUIRE(dst.data[0][(h - 1 - 20) * dst.stride[0] + (w - 1 - 10)] == 200); + REQUIRE(dst.data[1][(h / 2 - 1 - 30) * dst.stride[1] + (w / 2 - 1 - 40)] == 210); + REQUIRE(dst.data[2][(h / 2 - 1 - 50) * dst.stride[2] + (w / 2 - 1 - 60)] == 220); + } + + SECTION("rotate 270") { + image.Rotate(270); + REQUIRE(image.Width() == static_cast(h)); + REQUIRE(image.Height() == static_cast(w)); + + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, h, w); + // 270deg: (sx, sy) -> (dx, dy) = (sy, src_w-1-sx) + REQUIRE(dst.data[0][(w - 1 - 10) * dst.stride[0] + 20] == 200); + REQUIRE(dst.data[1][(w / 2 - 1 - 40) * dst.stride[1] + 30] == 210); + REQUIRE(dst.data[2][(w / 2 - 1 - 60) * dst.stride[2] + 50] == 220); + } +} + +TEST_CASE("Image::Rotate YUV420P odd dimensions", "[image]") { + bootstrap_image_config(); + // Odd luma dims exercise the ceiling in the chroma plane size: 101x57 + // luma -> 51x29 chroma. + const int w = 101, h = 57; + Image image(w, h, ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUV420P); + + Planes src = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + const int cw = (w + 1) / 2, ch = (h + 1) / 2; + memset(src.data[0], 0x10, static_cast(src.stride[0]) * h); + memset(src.data[1], 0x20, static_cast(src.stride[1]) * ch); + memset(src.data[2], 0x30, static_cast(src.stride[2]) * ch); + // Marker in the LAST chroma column/row - the part flooring would drop + src.data[1][(ch - 1) * src.stride[1] + (cw - 1)] = 211; + + image.Rotate(90); + REQUIRE(image.Width() == static_cast(h)); + REQUIRE(image.Height() == static_cast(w)); + + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, h, w); + // (cw-1, ch-1) -> (ch-1-(ch-1), cw-1) = (0, cw-1) + REQUIRE(dst.data[1][(cw - 1) * dst.stride[1] + 0] == 211); +} + +TEST_CASE("Image::Flip YUV420P", "[image]") { + bootstrap_image_config(); + const int w = 640, h = 480; + Image image(w, h, ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUV420P); + + Planes src = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + memset(src.data[0], 0x10, static_cast(src.stride[0]) * h); + memset(src.data[1], 0x20, static_cast(src.stride[1]) * (h / 2)); + memset(src.data[2], 0x30, static_cast(src.stride[2]) * (h / 2)); + src.data[0][20 * src.stride[0] + 10] = 200; + src.data[1][30 * src.stride[1] + 40] = 210; + + SECTION("horizontal") { + image.Flip(true); + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + REQUIRE(dst.data[0][20 * dst.stride[0] + (w - 1 - 10)] == 200); + REQUIRE(dst.data[1][30 * dst.stride[1] + (w / 2 - 1 - 40)] == 210); + } + + SECTION("vertical") { + image.Flip(false); + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + REQUIRE(dst.data[0][(h - 1 - 20) * dst.stride[0] + 10] == 200); + REQUIRE(dst.data[1][(h / 2 - 1 - 30) * dst.stride[1] + 40] == 210); + } +}