From 67417727e75549d11d713df67a4e4f63e849adae Mon Sep 17 00:00:00 2001 From: Si Date: Fri, 18 Sep 2026 16:49:44 +0200 Subject: [PATCH] Check the canvas size before narrowing it to an int MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A canvas side was cast to int with no range check, so a number that did not fit came back as one that did: `-s 4294968296x600` rendered a perfectly plausible 1000x600, and a hand-edited project file could ask for a width the reader turned into something else (or, for a double out of range, into undefined behaviour). Both front-ends now check the side before the cast, against one shared bound in chain.hpp — the 1..20000 the GUI spin buttons already offered, so the help text, the CLI error, the project reader and the interface all move together. The tests gain a `canvas size` section covering the round trip at both ends of the range and the refusals on either side of it, through the project reader and through `-s`; the test binary picks up the CLI parser to do it, which stays GUI-free. Co-Authored-By: Claude Opus 5 --- src/cli/options.cpp | 12 ++++-- src/core/chain.hpp | 7 ++++ src/core/project.cpp | 20 ++++++++-- src/gui/window.cpp | 4 +- tests/CMakeLists.txt | 4 +- tests/core_tests.cpp | 91 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 129 insertions(+), 9 deletions(-) diff --git a/src/cli/options.cpp b/src/cli/options.cpp index 504a159..6b24f41 100644 --- a/src/cli/options.cpp +++ b/src/cli/options.cpp @@ -24,7 +24,10 @@ bool parseSize(const std::string& text, int& width, int& height) { long w = 0; long h = 0; if (!parseLong(text.substr(0, sep), w) || !parseLong(text.substr(sep + 1), h)) return false; - if (w <= 0 || h <= 0) return false; + // Checked as longs, before the narrowing: 4294968296 cast straight to int + // is 1000, which would render a plausible canvas nobody asked for. + if (w < kMinCanvasSide || w > kMaxCanvasSide) return false; + if (h < kMinCanvasSide || h > kMaxCanvasSide) return false; width = static_cast(w); height = static_cast(h); return true; @@ -66,7 +69,9 @@ Options parseOptions(int argc, char** argv) { } else if (arg == "-s" || arg == "--size") { const std::string value = requireValue(argc, argv, i, arg); if (!parseSize(value, options.width, options.height)) { - throw EffectError("invalid size '" + value + "', expected e.g. 1920x1080"); + throw EffectError("invalid size '" + value + "', expected e.g. 1920x1080 with " + "each side between " + std::to_string(kMinCanvasSide) + " and " + + std::to_string(kMaxCanvasSide)); } } else if (arg == "--seed") { const std::string value = requireValue(argc, argv, i, arg); @@ -131,7 +136,8 @@ std::string usage(const char* programName) { << "Options:\n" << " -e, --effect NAME effect to apply, repeat to cascade (default: " << defaultEffectName() << ")\n" << " -o, --output FILE render headlessly to this file and exit\n" - << " -s, --size WxH canvas size (default: 1920x1080)\n" + << " -s, --size WxH canvas size, each side " << kMinCanvasSide << "-" + << kMaxCanvasSide << " (default: 1920x1080)\n" << " --seed N random seed (default: random)\n" << " --list-effects list available effects and exit\n" << " -h, --help show this help and exit\n"; diff --git a/src/core/chain.hpp b/src/core/chain.hpp index 72d95ef..420470b 100644 --- a/src/core/chain.hpp +++ b/src/core/chain.hpp @@ -10,6 +10,13 @@ namespace effector { +// The canvas sides both front-ends accept. The upper bound is not a hardware +// limit but the point past which a side stops fitting comfortably in an int +// once multiplied out, so both the CLI and the project reader can reject a +// number rather than silently wrap it into a plausible-looking one. +inline constexpr int kMinCanvasSide = 1; +inline constexpr int kMaxCanvasSide = 20000; + // One link of a cascade: an effect together with the values it runs with. struct ChainStage { EffectPtr effect; diff --git a/src/core/project.cpp b/src/core/project.cpp index d0de8c7..2712ab5 100644 --- a/src/core/project.cpp +++ b/src/core/project.cpp @@ -283,6 +283,21 @@ std::string relativeTo(const std::string& path, const std::string& directory) { return text.rfind("..", 0) == 0 ? path : text; } +// A canvas side is checked as a double, before it is narrowed: casting a +// number outside the range of an int is undefined, and even where it wraps +// quietly it turns a size the file never named into one that looks ordinary. +int canvasSide(const Json& value, const char* what) { + if (value.kind != Json::Kind::Number || value.number != std::trunc(value.number)) { + throw EffectError(std::string("the canvas ") + what + " must be a whole number"); + } + if (!(value.number >= kMinCanvasSide) || !(value.number <= kMaxCanvasSide)) { + throw EffectError(std::string("the canvas ") + what + " must be between " + + std::to_string(kMinCanvasSide) + " and " + + std::to_string(kMaxCanvasSide) + " pixels"); + } + return static_cast(value.number); +} + std::string resolve(const std::string& path, const std::string& directory) { std::filesystem::path file(path); if (file.is_absolute() || directory.empty()) return path; @@ -350,12 +365,11 @@ Project projectFromJson(const std::string& text, const std::string& directory) { Project project; if (const auto* canvas = root.member("canvas")) { - if (const auto* width = canvas->member("width")) project.width = static_cast(width->number); + if (const auto* width = canvas->member("width")) project.width = canvasSide(*width, "width"); if (const auto* height = canvas->member("height")) { - project.height = static_cast(height->number); + project.height = canvasSide(*height, "height"); } } - if (project.width <= 0 || project.height <= 0) throw EffectError("the canvas size is not positive"); if (const auto* seed = root.member("seed")) { if (seed->kind == Json::Kind::Number) { diff --git a/src/gui/window.cpp b/src/gui/window.cpp index 5c20daf..c46a393 100644 --- a/src/gui/window.cpp +++ b/src/gui/window.cpp @@ -176,10 +176,10 @@ Gtk::Widget& EffectorWindow::buildSidebar() { auto* grid = Gtk::make_managed(); grid->set_column_spacing(8); grid->set_row_spacing(6); - widthSpin_.set_range(1, 20000); + widthSpin_.set_range(kMinCanvasSide, kMaxCanvasSide); widthSpin_.set_increments(1, 100); widthSpin_.set_value(1920); - heightSpin_.set_range(1, 20000); + heightSpin_.set_range(kMinCanvasSide, kMaxCanvasSide); heightSpin_.set_increments(1, 100); heightSpin_.set_value(1080); for (auto* spin : {&widthSpin_, &heightSpin_}) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5fcf2aa..e98d7bd 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,4 +1,6 @@ -add_executable(effector_tests core_tests.cpp) +# The CLI parser joins the core here: it is GUI-free too, and the canvas size +# it produces has to obey the same bounds a project file does. +add_executable(effector_tests core_tests.cpp ../src/cli/options.cpp) target_link_libraries(effector_tests PRIVATE effector_core) target_compile_options(effector_tests PRIVATE -Wall -Wextra -Wpedantic) diff --git a/tests/core_tests.cpp b/tests/core_tests.cpp index 24f1b03..eb4541b 100644 --- a/tests/core_tests.cpp +++ b/tests/core_tests.cpp @@ -7,9 +7,12 @@ #include #include #include +#include #include +#include #include +#include "cli/options.hpp" #include "core/chain.hpp" #include "core/project.hpp" #include "core/registry.hpp" @@ -44,6 +47,28 @@ effector::Image gradient(int width, int height, int shade) { return image; } +// A canvas taken from a project file, written by hand rather than by +// projectToJson, so a size no writer would produce can be fed to the reader. +std::string projectWithCanvas(const std::string& width, const std::string& height) { + return "{\"canvas\": {\"width\": " + width + ", \"height\": " + height + + "},\n \"chain\": [{\"effect\": \"blur\"}]}"; +} + +// The canvas a `-s` value ends up as, or nothing when the parser refuses it. +std::optional> canvasFromSize(const std::string& size) { + char program[] = "effector"; + char flag[] = "-s"; + std::vector value(size.begin(), size.end()); + value.push_back('\0'); + char* argv[] = {program, flag, value.data()}; + try { + const auto options = effector::parseOptions(3, argv); + return std::pair{options.width, options.height}; + } catch (const effector::EffectError&) { + return std::nullopt; + } +} + bool sameBytes(const effector::Image& a, const effector::Image& b) { if (a.width() != b.width() || a.height() != b.height()) return false; return std::equal(a.data(), a.data() + a.byteSize(), b.data()); @@ -430,6 +455,72 @@ int main() { std::filesystem::remove_all(dir); } + std::cout << "canvas size\n"; + { + // The sizes the front-ends actually offer make the round trip whole. + int kept = 0; + for (const auto [w, h] : {std::pair{1, 1}, + {640, 400}, + {1920, 1080}, + {kMaxCanvasSide, kMaxCanvasSide}}) { + const auto back = projectFromJson( + projectWithCanvas(std::to_string(w), std::to_string(h))); + if (back.width == w && back.height == h) ++kept; + } + check(kept == 4, "every canvas in range is read back exactly"); + + Project project; + project.stages.push_back({effectByName("blur"), {}}); + project.width = kMaxCanvasSide; + project.height = 3; + const auto written = projectFromJson(projectToJson(project)); + check(written.width == kMaxCanvasSide && written.height == 3, + "the largest canvas survives a write and a read"); + + // A side that does not fit in an int must not be narrowed into one + // that does: 4294968296 truncates to a perfectly plausible 1000. + int refusals = 0; + for (const char* bad : {"4294968296", "2147483648", "99999999999999999999", "-5", "0", + "20001", "800.5", "\"800\"", "true"}) { + try { + projectFromJson(projectWithCanvas(bad, "600")); + } catch (const EffectError&) { + ++refusals; + } + } + check(refusals == 9, "a canvas width an int cannot hold is refused, not wrapped"); + + refusals = 0; + for (const char* bad : {"4294968296", "-5", "0", "20001", "800.5"}) { + try { + projectFromJson(projectWithCanvas("800", bad)); + } catch (const EffectError&) { + ++refusals; + } + } + check(refusals == 5, "the same bounds apply to the canvas height"); + + // A canvas the file does not mention keeps the defaults rather than + // failing: an old project is still readable. + const auto bare = projectFromJson("{\"chain\": [{\"effect\": \"blur\"}]}"); + check(bare.width == 1920 && bare.height == 1080, "a project with no canvas keeps 1920x1080"); + + // The CLI narrows the same way, from a long rather than a double. + const auto plain = canvasFromSize("800x600"); + check(plain && plain->first == 800 && plain->second == 600, "-s 800x600 is stored as given"); + const auto big = canvasFromSize(std::to_string(kMaxCanvasSide) + "x" + + std::to_string(kMaxCanvasSide)); + check(big && big->first == kMaxCanvasSide, "-s takes the largest canvas"); + check(!canvasFromSize("4294968296x600"), "-s refuses a width that does not fit in an int"); + check(!canvasFromSize("2147483648x600"), "-s refuses a width just past INT_MAX"); + check(!canvasFromSize("99999999999999999999x600"), "-s refuses a width past a long"); + check(!canvasFromSize("800x4294968296"), "-s refuses a height that does not fit in an int"); + check(!canvasFromSize("20001x600") && !canvasFromSize("800x20001"), + "-s refuses a side past the canvas limit"); + check(!canvasFromSize("0x600") && !canvasFromSize("-5x600"), + "-s refuses a side that is not positive"); + } + std::cout << "image io\n"; { const auto path = std::filesystem::temp_directory_path() / "effector_test_roundtrip.png";