From 7703228e1a0cc9c35858c06be1ee5dac7f2ee8e7 Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 01:49:06 +0000 Subject: [PATCH 1/6] build & test w/ sanitizers --- .github/workflows/build.yml | 12 +++++++++--- CMakeLists.txt | 9 +++++++++ README.md | 20 ++++++++++++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5f7fbdb..fdbbdc4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -44,8 +44,14 @@ jobs: Release, Debug, ] + sanitizer: [ + none, + address, + thread, + undefined, + ] runs-on: ${{ matrix.setup.os }} - name: ${{ matrix.setup.os }}-${{ matrix.setup.build }}-${{ matrix.type }} + name: ${{ matrix.setup.os }}-${{ matrix.setup.build }}-${{ matrix.type }}-sanitizer-${{ matrix.sanitizer }} timeout-minutes: 30 steps: @@ -58,7 +64,7 @@ jobs: - name: ccache uses: hendrikmuhs/ccache-action@v1.2.11 with: - key: ${{ matrix.setup.os }}-${{ matrix.setup.build }}-${{ matrix.type }} + key: ${{ matrix.setup.os }}-${{ matrix.setup.build }}-${{ matrix.type }}-sanitizer-${{ matrix.sanitizer }}-ccache - name: Set up CMake uses: lukka/get-cmake@latest @@ -75,7 +81,7 @@ jobs: - name: Configure CMake env: HF_TOKEN: ${{ secrets.HF_TOKEN }} - run: cmake -B ${{github.workspace}}/build ${{ matrix.setup.defines }} -DCMAKE_BUILD_TYPE=${{ matrix.type }} + run: cmake -B ${{github.workspace}}/build ${{ matrix.setup.defines }} -DCMAKE_BUILD_TYPE=${{ matrix.type }} -DMINJA_SANITIZER=${{ matrix.sanitizer }} - name: Build run: cmake --build ${{github.workspace}}/build --config ${{ matrix.type }} --parallel diff --git a/CMakeLists.txt b/CMakeLists.txt index 95dabe7..b92ae02 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -43,6 +43,15 @@ option(MINJA_EXAMPLE_ENABLED "minja: Build with example" option(MINJA_FUZZTEST_ENABLED "minja: fuzztests enabled" MINJA_FUZZTEST_ENABLED_DEFAULT) option(MINJA_FUZZTEST_FUZZING_MODE "minja: run fuzztests (if enabled) in fuzzing mode" OFF) option(MINJA_USE_VENV "minja: use Python venv for build" MINJA_USE_VENV_DEFAULT) +set(MINJA_SANITIZERS thread address undefined none) +set(MINJA_SANITIZER none CACHE STRING "minja: sanitizer to use") +set_property(CACHE MINJA_SANITIZER PROPERTY STRINGS ${MINJA_SANITIZERS}) + +if (NOT MSVC AND NOT MINJA_SANITIZER STREQUAL "none") + message(STATUS "Using -fsanitize=${MINJA_SANITIZER}") + add_compile_options("-fsanitize=${MINJA_SANITIZER}") + link_libraries ("-fsanitize=${MINJA_SANITIZER}") +endif() set(CMAKE_CXX_STANDARD 17) diff --git a/README.md b/README.md index 5981079..36a3291 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,26 @@ Main limitations (non-exhaustive list): ./scripts/fuzzing_tests.sh ``` +- Sanitizer tests: + + ```bash + for sanitizer in ADDRESS THREAD UNDEFINED ; do + docker run --rm \ + -v "$PWD":/src:ro \ + -v "$PWD/build-sanitizer-${sanitizer}":/src/build \ + -w /src \ + "$(echo " + FROM ghcr.io/astral-sh/uv:debian-slim + RUN apt-get update && apt-get install -y build-essential libcurl4-openssl-dev cmake clang-tidy + " | docker build . -q -f - )" \ + bash -c " + cmake -B build -DCMAKE_BUILD_TYPE=Debug -DMINJA_SANITIZER=${sanitizer} && \ + cmake --build build -j --config Debug && \ + ctest --test-dir build -j -C Debug --output-on-failure + " + done + ``` + - If your model's template doesn't run fine, please consider the following before [opening a bug](https://github.com/googlestaging/minja/issues/new): - Is the template using any unsupported filter / test / method / global function, and which one(s)? From 8fb880f3cd29c361eef7851ba7c572445cf32e1a Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 01:51:50 +0000 Subject: [PATCH 2/6] Fix circular context reference docker run --rm \ -v "$PWD":/src:ro \ -v "$PWD/build-docker":/src/build \ -w /src \ "$(echo " FROM ghcr.io/astral-sh/uv:debian-slim RUN apt-get update && apt-get install -y build-essential libcurl4-openssl-dev cmake clang-tidy " | docker build . -q -f - )" \ bash -c " cmake -B build -DCMAKE_BUILD_TYPE=Debug -DMINJA_SANITIZER=address && \ cmake --build build -j --config Debug && \ ctest --test-dir build -j -C Debug --output-on-failure " --- include/minja/minja.hpp | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index 5ed0556..4295e73 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -1060,11 +1060,18 @@ class MacroNode : public TemplateNode { } } } - void do_render(std::ostringstream &, const std::shared_ptr & macro_context) const override { + void do_render(std::ostringstream &, const std::shared_ptr & context) const override { if (!name) throw std::runtime_error("MacroNode.name is null"); if (!body) throw std::runtime_error("MacroNode.body is null"); - auto callable = Value::callable([this, macro_context](const std::shared_ptr & call_context, ArgumentsValue & args) { - auto execution_context = Context::make(Value::object(), macro_context); + + // Use init-capture to avoid dangling 'this' pointer and circular references + auto callable = Value::callable([weak_context = std::weak_ptr(context), + name = name, params = params, body = body, + named_param_positions = named_param_positions] + (const std::shared_ptr & call_context, ArgumentsValue & args) { + auto context_locked = weak_context.lock(); + if (!context_locked) throw std::runtime_error("Macro context no longer valid"); + auto execution_context = Context::make(Value::object(), context_locked); if (call_context->contains("caller")) { execution_context->set("caller", call_context->get("caller")); @@ -1640,13 +1647,17 @@ class CallNode : public TemplateNode { void do_render(std::ostringstream & out, const std::shared_ptr & context) const override { if (!expr) throw std::runtime_error("CallNode.expr is null"); if (!body) throw std::runtime_error("CallNode.body is null"); - - auto caller = Value::callable([this, context](const std::shared_ptr &, ArgumentsValue &) -> Value { - return Value(body->render(context)); + + // Use init-capture to avoid dangling 'this' pointer and circular references + auto caller = Value::callable([weak_context = std::weak_ptr(context), body=body] + (const std::shared_ptr &, ArgumentsValue &) -> Value { + auto context_locked = weak_context.lock(); + if (!context_locked) throw std::runtime_error("Caller context no longer valid"); + return Value(body->render(context_locked)); }); - + context->set("caller", caller); - + auto call_expr = dynamic_cast(expr.get()); if (!call_expr) { throw std::runtime_error("Invalid call block syntax - expected function call"); @@ -1657,7 +1668,7 @@ class CallNode : public TemplateNode { throw std::runtime_error("Call target must be callable: " + function.dump()); } ArgumentsValue args = call_expr->args.evaluate(context); - + Value result = function.call(context, args); out << result.to_str(); } @@ -2215,7 +2226,7 @@ class Parser { } } } - + if ((has_first_colon || has_second_colon)) { index = std::make_shared(slice_loc, std::move(start), std::move(end), std::move(step)); } else { From 3d5223c7adf8f9306d350ceeb2dd9e54d2ebcf20 Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 01:56:13 +0000 Subject: [PATCH 3/6] fix bad patch --- include/minja/minja.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index 4295e73..e861406 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -1101,7 +1101,7 @@ class MacroNode : public TemplateNode { } return body->render(execution_context); }); - macro_context->set(name->get_name(), callable); + context->set(name->get_name(), callable); } }; @@ -1271,7 +1271,7 @@ class SubscriptExpr : public Expression { } return result; - } else if (target_value.is_array()) { + } else if (target_value.is_array()) { auto result = Value::array(); for (int64_t i = start; step > 0 ? i < end : i > end; i += step) { result.push_back(target_value.at(i)); @@ -1320,7 +1320,7 @@ static bool in(const Value & value, const Value & container) { return (((container.is_array() || container.is_object()) && container.contains(value)) || (value.is_string() && container.is_string() && container.to_str().find(value.to_str()) != std::string::npos)); -}; +} class BinaryOpExpr : public Expression { public: From b56da72abbd96e812854c40d6ab0b56140f11436 Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 02:00:11 +0000 Subject: [PATCH 4/6] Add tiny reserves in value ctor (+ use emplace to avoid some copies) --- include/minja/minja.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index 5ed0556..dc2e134 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -158,12 +158,14 @@ class Value : public std::enable_shared_from_this { Value(const json & v) { if (v.is_object()) { auto object = std::make_shared(); + object->reserve(v.size()); for (auto it = v.begin(); it != v.end(); ++it) { - (*object)[it.key()] = it.value(); + object->emplace_back(it.key(), Value(it.value())); } object_ = std::move(object); } else if (v.is_array()) { auto array = std::make_shared(); + array->reserve(v.size()); for (const auto& item : v) { array->push_back(Value(item)); } From 00538f9dc58adeba9c8dc6cc69c398f4b751f6a6 Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 02:00:21 +0000 Subject: [PATCH 5/6] drop unused enable_shared_from_this --- include/minja/minja.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index dc2e134..129fe11 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -55,7 +55,7 @@ inline std::string normalize_newlines(const std::string & s) { } /* Values that behave roughly like in Python. */ -class Value : public std::enable_shared_from_this { +class Value { public: using CallableType = std::function &, ArgumentsValue &)>; using FilterType = std::function &, ArgumentsValue &)>; @@ -612,7 +612,7 @@ static std::string error_location_suffix(const std::string & source, size_t pos) return out.str(); } -class Context : public std::enable_shared_from_this { +class Context { protected: Value values_; std::shared_ptr parent_; @@ -852,12 +852,12 @@ struct LoopControlTemplateToken : public TemplateToken { struct CallTemplateToken : public TemplateToken { std::shared_ptr expr; - CallTemplateToken(const Location & loc, SpaceHandling pre, SpaceHandling post, std::shared_ptr && e) + CallTemplateToken(const Location & loc, SpaceHandling pre, SpaceHandling post, std::shared_ptr && e) : TemplateToken(Type::Call, loc, pre, post), expr(std::move(e)) {} }; struct EndCallTemplateToken : public TemplateToken { - EndCallTemplateToken(const Location & loc, SpaceHandling pre, SpaceHandling post) + EndCallTemplateToken(const Location & loc, SpaceHandling pre, SpaceHandling post) : TemplateToken(Type::EndCall, loc, pre, post) {} }; @@ -1077,7 +1077,7 @@ class MacroNode : public TemplateNode { auto & arg = args.args[i]; if (i >= params.size()) throw std::runtime_error("Too many positional arguments for macro " + name->get_name()); param_set[i] = true; - auto & param_name = params[i].first; + const auto & param_name = params[i].first; execution_context->set(param_name, arg); } for (auto & [arg_name, value] : args.kwargs) { From f7e04ebfcea3ed9df192b22b5f3aa4cdec0edf51 Mon Sep 17 00:00:00 2001 From: ochafik Date: Wed, 29 Oct 2025 02:10:13 +0000 Subject: [PATCH 6/6] Update minja.hpp --- include/minja/minja.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index 129fe11..6583e3f 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -33,6 +33,7 @@ using json = nlohmann::ordered_json; + namespace minja { class Context;