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)? diff --git a/include/minja/minja.hpp b/include/minja/minja.hpp index 5ed0556..e82ff0b 100644 --- a/include/minja/minja.hpp +++ b/include/minja/minja.hpp @@ -33,6 +33,7 @@ using json = nlohmann::ordered_json; + namespace minja { class Context; @@ -55,7 +56,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 &)>; @@ -158,12 +159,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)); } @@ -610,7 +613,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_; @@ -850,12 +853,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) {} }; @@ -1060,11 +1063,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")); @@ -1075,7 +1085,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) { @@ -1094,7 +1104,7 @@ class MacroNode : public TemplateNode { } return body->render(execution_context); }); - macro_context->set(name->get_name(), callable); + context->set(name->get_name(), callable); } }; @@ -1264,7 +1274,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)); @@ -1313,7 +1323,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: @@ -1640,13 +1650,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 +1671,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 +2229,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 {