[v2] Use allocators internally instead of malloc/free and stop generating zero-terminated strings#418
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## anilm3/v2 #418 +/- ##
=============================================
- Coverage 85.99% 85.88% -0.12%
=============================================
Files 176 176
Lines 9063 9054 -9
Branches 3835 3853 +18
=============================================
- Hits 7794 7776 -18
- Misses 492 498 +6
- Partials 777 780 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmarks clangBenchmark execution time: 2025-07-02 12:13:08 Comparing candidate commit 29abeb0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
Benchmarks gccBenchmark execution time: 2025-07-02 12:16:46 Comparing candidate commit 29abeb0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
Benchmarks clang-pgoBenchmark execution time: 2025-07-02 12:29:54 Comparing candidate commit 29abeb0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
Anilm3
changed the title
7418204 to
1d40b34
Compare
Anilm3
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR begins adding allocator support internally by replacing malloc/free, removes zero-terminated-string handling, renames the core API, and adds object cloning.
- Renamed
ddwaf_runAPI toddwaf_context_evaland updated all callers and docs - Swapped raw
malloc/freeformemory::memory_resourceallocations indynamic_stringandcow_string - Stopped generating null-terminated buffers and introduced a
free_objectsflag andddwaf_object_clone
Reviewed Changes
Copilot reviewed 20 out of 77 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/exclusion/common.hpp | Removed unnecessary ddwaf.h include |
| src/dynamic_string.hpp | Migrated to memory_resource, removed null term logic |
| src/dynamic_string.cpp | Updated to_object() to use allocators and flag deallocation |
| src/cow_string.hpp | Swapped free for memory_resource, added capacity tracking |
| src/context_allocator.hpp | Switched from std::pmr::memory_resource to custom type |
| src/context.hpp | Renamed run→eval, added insert(), threaded allocator |
| src/context.cpp | Renamed implementation of run to eval |
| src/condition/scalar_condition.cpp | Simplified transform API usage |
| src/builder/waf_builder.hpp | Dropped free_fn parameter from constructor |
| src/builder/ruleset_builder.hpp/.cpp | Removed free_fn storage and assignment |
| smoketest/smoke.c | Updated test to call new ddwaf_context_eval API |
| libddwaf.def | Exported renamed API and new clone function |
| include/ddwaf.h | Updated interface signature, removed free_fn, added clone |
| fuzzer/global/src/interface.cpp | Updated config init and calls to new API |
| examples/example.cpp | Updated example to use renamed API |
| benchmark/run_fixture.cpp | Updated benchmark calls to new API |
| benchmark/main.cpp | Adjusted config initialization |
| README.md | Updated usage example to renamed API |
| BINDING_IMPL_NOTES.md | Updated documentation to renamed API |
cataphract
left a comment
cataphract
left a comment
There was a problem hiding this comment.
So is the idea to have three allocators?
- The context allocator, for libddwaf-side allocations whose lifetime is the context's.
- A general allocator for undifferentiated allocations.
- An output data allocator?
I see that you removed free_fn, which was controlled deallocation for the input. I think that's fine because it was not very useful: you either retained ownership, in which case you could yourself destroy the objects once you closed the context, or you passed ownership and let libddwaf do it. So a boolean suffices.
If the idea is to have a general "user allocator" that does:
- allocation of input via the ddwaf functions
- allocation of output
- deallocation of input (after run for ephemeral, after context close for persistent)
- deallocation of output (via call to ddwaf_function)
then I have reservations. It's not clear to me that having the same allocator for input and for output is useful. I also don't see how this is any better than the status quo:
- For input, we already have all the control we need (with free_fn = 0), provided we don't use the libddwaf functions to build the objects. Whether the libddwaf functions can be efficiently and ergonomically used a custom input allocator, that remains to be seen.
- For output, it's not clear to me that anyone would want to have their custom allocator. In Java, where the native memory is not unified (only certain byte buffers can be shared memory between the native side and the java side), I could see some advantages, as you would be able to read the output ddwaf objects directly from java if your allocator wrote in direct byte buffers. Though probably not worth it, unless the output objects get bigger. But in any case, I think you still want to distinguish between allocated input and output (their lifetimes are not the same for instance, so implementing a winkout deallocator that handles both at the same time is complicated)
Artifact Size Comparison 📦
|
|
As discussed offline, the idea is to have 3 allocators:
Eventually the plan is to expose allocators (memory resources), such as a monotonic and an unsynchronized pool one. The user allocator will still be an option but I don't know if it'll be of interest, I think there is more value in being able to choose from an existing one to optimise your allocations around your concurrency model, than being able to allocate memory from your runtime. |
* Object view: read only abstraction to ddwaf_object (#341) * [v2] Remove mingw builds (#381) * Writable objects: owned and borrowed object and object limits removal (#378, #382) * [v2] Refactor and improve object types (#387) * [v2] Update unit tests to use new abstractions (#389) * [v2] Update `raw_configuration` type to use `object_view` (#390) * [v2] Remove remaining uses of ddwaf_object in `src` and `tests/unit` (#391) * [v2] JWT Decoding Processor (#401) * [v2] First iteration of object layout changes (#394) * Split context data insertion from evaluation (#407) * [v2] Second iteration of object layout changes (#408) * [v2] Add new fingerprint and object view tests (#414) * Reenable attribute collector unit test (#415) * [v2] Container view types (#413) * Exclude assertions from coverage (#416) * [v2] Use allocators internally instead of malloc/free and stop generating zero-terminated strings (#418) * Add memory resource to owned and borrowed objects (#428) * [v2] Propagate allocators from context (#420) * [v2] Update interface and expose allocators (#427) * Refactor evaluation stages out of the context (#442) * Subcontext: replace ephemerals with a new scope with user-defined lifetime derived from the context (#443) * Validator: Add support for testing subcontexts and attributes (#451) * [v2] Pass allocator to context and subcontext eval and add new allocators (#452) * Update logger to avoid dependencies on ddwaf.h (#453) * [v2] Return DDWAF_MATCH when there are events, attributes or actions (#455) * [v2] Cleanup: remove exclusion namespace and some redundant references (#456)
This is the first PR in the process to introduce allocators, in a limited fashion, to the
libddwafinterface. The main changes done here are the following:ddwaf_runtoddwaf_context_eval.ddwaf_object_freefromddwaf_config, as a consequence benchmarks were failing so a temporaryfree_objectsboolean has been added toddwaf_context_evaluntil allocators can be passed to the context.malloc/freehave been replaced with the use of astd::pmr::new_delete_resource. With this change, all allocated memory is accounted for and no capacity is needed for output strings.ddwaf_object_clonehas been added to the interface.The use of allocators is rudimentary as the goal is to propagate an allocator directly from the context, rather than getting a default one, however the PR is already relatively large so those changes will be done in the next PR.
Remaining work (Next PRs):
context_init.ddwaf_objectfunctions.