Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v2] Writable objects: owned and borrowed object #378

Merged
merged 129 commits into from
Mar 20, 2025

Conversation

Anilm3
Copy link
Collaborator

@Anilm3 Anilm3 commented Mar 9, 2025

This PR introduces two new object types:

  • owned_object: as the name suggests, this is an object which owns its own memory. This abstraction ensures that the memory associated with an object is freed on destruction and moved appropriately.
  • borrowed_object: an object type which points to memory owned by a different object, this allows accessing elements from an owned_object without the risk of accidental double-free.

In addition, some code from object_view has been refactored to ensure that owned_object and borrowed_object are both writable and readable. More of object_view will be refactored in the next few PRs.

Some quirks on this PR:

  • The public functions ddwaf_object_* still contain a separate implementation, these will be updated to use the new abstractions in a following PR. This code has been moved from object.cpp to interface.cpp without changes.
  • owned_object and borrowed_object currently don't check for free function compatibility. While incorrect, this shouldn't be an issue at the moment and free functions are being replaced by allocators in a subsequent PR.

Remaining work (future PRs):

  • Replace all uses of ddwaf_object within tests with owned_object, borrowed_object and object_view.
  • Completely refactor object_view and object_converter.
  • Remove raw_configuration in favour of object_view or use object_view behind the scenes.
  • Update ddwaf_object_* interface to use new types.
  • Improve context method to avoid ddwaf_object arguments or optional_ref.
  • Add allocator support and check for allocator compatibility on emplace and emplace_back.
  • Remove all uses of malloc / free

@Anilm3 Anilm3 changed the title [WIP] Writable objects: owned and borrowed object Writable objects: owned and borrowed object Mar 16, 2025
@Anilm3 Anilm3 marked this pull request as ready for review March 16, 2025 15:55
@Anilm3 Anilm3 requested a review from a team as a code owner March 16, 2025 15:55
@Anilm3 Anilm3 changed the title Writable objects: owned and borrowed object [v2] Writable objects: owned and borrowed object Mar 16, 2025
@DataDog DataDog deleted a comment from pr-commenter bot Mar 16, 2025
ddwaf_object_array_add(&array, ddwaf_object_stringl(&tmp, arg.data(), arg.size()));
}
auto root = owned_object::make_map();
root.emplace("server.request.query", owned_object::make_string(param));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps you could add some syntax sugar like:

#include <string_view>

struct kv;
struct dw_obj {
    char _c{};

    dw_obj(dw_obj &&) = default;
    dw_obj(const dw_obj &) = delete;
    dw_obj &operator=(dw_obj &&) = default;
    dw_obj &operator=(const dw_obj &) = delete;
    ~dw_obj() = default;

    dw_obj(int i) {} // NOLINT
    dw_obj(std::string_view) {} // NOLINT
    template<typename Str> requires std::convertible_to<Str, std::string_view>
    dw_obj(Str) {} // NOLINT
    dw_obj(std::initializer_list<kv>) {} // NOLINT
    dw_obj(std::initializer_list<dw_obj>) {} // NOLINT

    protected:
    dw_obj() = default;
};
struct kv : std::pair<std::string_view, dw_obj> {
    using std::pair<std::string_view, dw_obj>::pair;
    kv(const std::string_view& sv, dw_obj&& obj) : std::pair<std::string_view, dw_obj>{
        sv, std::move(obj)
    } {} // NOLINT
};

void test() {
    dw_obj map{
        kv{"foo", {kv{"bar", "foo"}}},
        kv{
            "xxx",
            {
                1,
                3,
                "yyy", 
                {{"zzz"}},
                {kv{"aaa", "bbb"}}}}};
}

compiler explorer

Empty arrays/maps need special handling; you could add constants, or you could add dw_obj_array, dw_obj_map objects etc. You could use a similar model to that of nginx: they derive dw_obj, they have may_alias, and you can (behind an optional for safe conversions) convert borrowed dw_obj into dw_obj_map/etc. and unconditionally convert in the other direction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I had some thoughts along these lines as well, I'll have a look at what you've done and see if I can introduce anything here as well in the next PR (before I update all tests....)

@Anilm3 Anilm3 requested a review from cataphract March 17, 2025 17:23
@pr-commenter
Copy link

pr-commenter bot commented Mar 19, 2025

Benchmarks clang-pgo

Benchmark execution time: 2025-03-19 12:12:32

Comparing candidate commit c6d9fbf in PR branch anilm3/writable_objects with baseline commit f3f23ee in branch anilm3/v2.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

@Anilm3 Anilm3 merged commit 6f1e0d9 into anilm3/v2 Mar 20, 2025
43 of 46 checks passed
@Anilm3 Anilm3 deleted the anilm3/writable_objects branch March 20, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants