Remove default allocator arguments #486
Open
+2,198
−1,661
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The default allocator argument caused runtime exceptions "emplace: incompatible allocators" (from writable_object::emplace/emplace_back in src/object.hpp:1179, 1216) when converting a dynamic_string to an object with a mismatched allocator. By making the allocator parameter required, such bugs will be caught at compile time instead of causing runtime exceptions. This change fixes two instances in fingerprint.cpp where the allocator was not being passed, which triggered these allocator mismatch exceptions, and updates tests to explicitly pass allocators. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
Dynamic Artifact Size Comparison 📦
Static Artifact Size Comparison 📦
|
Benchmarks clang-pgoBenchmark execution time: 2026-01-01 23:04:02 Comparing candidate commit a9635e6 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
This change extends the allocator refactoring to comprehensively prevent allocator mismatch bugs by: 1. **Remove default allocators from owned_object constructors** - String constructors now require explicit allocator parameter - Prevents implicit use of default allocator when specific one needed - Compile-time enforcement of correct allocator usage 2. **Privatize raw constructor, expose via create_unchecked()** - `owned_object(detail::object, alloc)` now private - Added `create_unchecked()` static factory to make danger explicit - Updated call sites in interface.cpp and elsewhere 3. **Use null allocator for primitives** - Primitives (null, boolean, numbers) use memory::get_default_null_resource() - Null allocator throws if allocation attempted, catching bugs early 4. **Rename unsafe methods for clarity** - `make_string_nocopy` → `unsafe_make_string_nocopy` - Explicit naming indicates caller must ensure memory ownership 5. **Template improvements using std::constructible_from** - attribute_collector insert() overloads use concept constraints - writable_object emplace/emplace_back use std::constructible_from - Cleaner distinction between allocating and non-allocating types 6. **Test infrastructure improvements** - Added tests/common/ddwaf_object_da.hpp for test allocator utilities - Updated all test files to use new constructor signatures 7. **Fix examples and benchmarks for current API** - examples/example.cpp: Updated to use ddwaf_object_set_* functions - tools/ip_match_benchmark.cpp: Fixed deprecated API and IPv6 code - tools/infer_schema_bench.cpp: Updated include path (still needs API rewrite) All 1,513 tests pass with zero memory leaks (51,813 allocs/deallocs balanced). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cataphract
changed the title
Contributor
Author
|
There were more similar bugs. Added another commit to remove more default allocator parameters |
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
cataphract
force-pushed
the
glopes/fix-incompatible-allocators
branch
from
a734927 to
358fa45
Compare
There was a problem hiding this comment.
Pull request overview
This pull request removes default allocator arguments from the owned_object API to enforce compile-time checking of allocator compatibility, preventing runtime "incompatible allocators" exceptions. The change makes allocator parameters required for operations that need memory management, ensuring that allocator mismatches are caught during compilation rather than at runtime.
- Fixed two bugs in
fingerprint.cppwhere allocators were not being passed todynamic_string::to_object() - Introduced a test helper class
ddwaf_object_dathat provides convenience wrappers with default allocators for test code - Updated all production code to explicitly pass allocators where required
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/object.hpp | Removed default allocator parameters from constructors and factory methods; added create_unchecked() for unsafe operations; updated template constraints to use std::constructible_from |
| src/dynamic_string.hpp | Made allocator parameter required in to_object() method signature |
| src/dynamic_string.cpp | Updated to_object() implementation to use required allocator parameter with proper safety comments |
| src/processor/fingerprint.cpp | Fixed two instances where allocator was missing from buffer.to_object() calls |
| src/serializer.cpp | Added allocator parameter to collect_attributes() and passed through to collector.insert() |
| src/ruleset_info.hpp | Updated section_info and to_object() to explicitly pass default resource allocator |
| src/ruleset_info.cpp | Updated to pass allocator when creating arrays in add_failed() |
| src/json_utils.cpp | Updated string creation to pass allocator explicitly |
| src/interface.cpp | Updated to use create_unchecked() for unsafe operations; added safety comments |
| src/attribute_collector.hpp | Added overload for inserting values that require allocators |
| tests/common/ddwaf_object_da.hpp | New test helper class providing factory methods with default allocators |
| tests/common/yaml_utils.cpp | Updated YAML conversion to use test helper methods |
| tests/unit/*.cpp | Updated all test files to use test::ddwaf_object_da helper methods instead of direct constructors |
| tools/ip_match_benchmark.cpp | Updated to explicitly pass allocators and use new API functions |
| tools/infer_schema_bench.cpp | Added allocator variable and updated namespace reference |
| examples/example.cpp | Updated example to use new API with explicit allocators and pointer-based insert functions |
| fuzzer/**/src/main.cpp | Updated all fuzzers to explicitly obtain and pass default allocator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
cataphract
force-pushed
the
glopes/fix-incompatible-allocators
branch
from
358fa45 to
617ff3d
Compare
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
cataphract
force-pushed
the
glopes/fix-incompatible-allocators
branch
from
617ff3d to
998f840
Compare
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
* Remove public constructors from owned_object, as these just duplicate ddwaf_object::make_* factory methods and may have confusing affects around some implicit conversions. * make borrowed objects destroy the current value when they're assigned to from an owned_object&&. This prevents some usages of to_borrowed(x), where x is uninitialized memory. * remove more implicit usages of the default allocator, causing runtime errors.
cataphract
force-pushed
the
glopes/fix-incompatible-allocators
branch
from
998f840 to
a9635e6
Compare
cataphract
had a problem deploying
to
dd-protected-coverage
GitHub Actions
Failure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The default allocator argument caused runtime exceptions "emplace: incompatible allocators" (from writable_object::emplace/emplace_back in src/object.hpp:1179, 1216) when converting a dynamic_string to an object with a mismatched allocator.
By making the allocator parameter required, such bugs will be caught at compile time instead of causing runtime exceptions.
This change fixes two instances in fingerprint.cpp where the allocator was not being passed, which triggered these allocator mismatch exceptions, and updates tests to explicitly pass allocators.