`before_send` event modifications leak to the scope

[jpnurmi]

Description

When before_send modifies values on an event, the changes can leak back into the scope.

• user and fingerprint are placed into the event by reference (PLACE_VALUE) rather than being cloned, so any modification corrupts the scope directly.
• tags, extra, and contexts use PLACE_CLONED_VALUE, but sentry__value_clone() only performs a shallow clone so modifying a nested object still mutates the scope's original.

When does the problem happen

• During build
• During run-time
• When capturing a hard crash

Steps To Reproduce

#include <stdio.h>
#include <sentry.h>

sentry_value_t before_send(sentry_value_t event, void *hint, void *user_data)
{
    sentry_value_t user = sentry_value_get_by_key(event, "user");
    char* original = sentry_value_to_json(user);

    sentry_value_set_by_key(user, "email", sentry_value_new_string("redacted"));
    char* redacted = sentry_value_to_json(user);

    fprintf(stderr, "%s ==> %s\n", original, redacted);
    sentry_free(original);
    sentry_free(redacted);

    return event;
}

int main(int argc, char *argv[])
{
    sentry_options_t *options = sentry_options_new();
    sentry_options_set_dsn(options, SENTRY_DSN);
    sentry_options_set_before_send(options, before_send, nullptr);
    sentry_init(options);

    sentry_set_user(sentry_value_new_user(NULL, "somebody", "somebody@example.com", NULL));

    sentry_capture_event(sentry_value_new_event()); // {"username":"somebody","email":"somebody@example.com"} ==> {"username":"somebody","email":"redacted"}
    sentry_capture_event(sentry_value_new_event()); // {"username":"somebody","email":"redacted"} ==> {"username":"somebody","email":"redacted"}

    sentry_close();
}

Log output

Expected output:

{"username":"somebody","email":"somebody@example.com"} ==> {"username":"somebody","email":"redacted"}
{"username":"somebody","email":"somebody@example.com"} ==> {"username":"somebody","email":"redacted"}

Actual output:

{"username":"somebody","email":"somebody@example.com"} ==> {"username":"somebody","email":"redacted"}
{"username":"somebody","email":"redacted"} ==> {"username":"somebody","email":"redacted"}

The second before_send receives "redacted" as the original email because the first before_send mutated the scope's user object in place.

[linear]

NATIVE-171

[supervacuus]

Tbh, while this is clearly a bug in the context of the hooks, the difference in handling between values like user or fingerprint and other scope content exists because of expected concurrent modification while serializing. user is always updated atomically as the entire object via the API, whereas tags will be modified by key.

When the event is passed to the hooks, we suddenly break this contract, and the internal structure leaks through. sentry__scope_apply_to_event doesn't at all consider the change in semantics when an applied-to event suddenly is entirely mutable. In that case, the right choice is not to change the behavior in sentry__scope_apply_to_event, but to actually pass a deep clone of the event to the hooks, right before we invoke them.

Since we give users the entire object-graph and they are free to mutate everything, there is no other way to restrict access to shared values. It also adds the cost of the deep clone where needed, rather than having everyone pay for it, and implementers of the hook don't have to scratch their heads about arbitrary mutation boundaries.

[jpnurmi]

Yeah, I realized in #1550 that deep clones of large event objects would get costly. One option could be copy-on-write to minimize the cost for non-mutating hooks, and the existing thing_t indirection for lists and objects should make it fairly doable. What do you think, would COW be an acceptable approach here?

[supervacuus]

Yeah, I realized in #1550 that deep clones of large event objects would get costly.

I mean, it is entirely unclear to me how prevalent large event objects are, but yes, every user who registers a hook would have to pay for it, even with only read accesses or even no event access inside the hook, and there is certainly a worst-case effect.

What do you think, would COW be an acceptable approach here?

COW could certainly be a replacement for clone, but it has to be designed carefully:

• it must be optional (because otherwise it would break stuff and also introduce cost where we don't need it), so we need a function like sentry__value_cow_snapshot() that acts analogous to sentry__value_clone()
• that function would return a value that contains a new thing type (cow_proxy_t or whatever), and that thing points to a target thing to the shared original (and the proxy would only be applied for mutable thing types, like list and object, inline values are returned immediately, and immutable ref values would just be increfed)
• but(!!!) it must also provide a way to track the thing's children! The problem with COW is that we can't do it purely at the thing level; we must materialize the children container on first access to any child, because otherwise there is no place to write your copy back without mutating the value you actually want to protect.

so the sequencing would be like this:

// allocates a new cow_proxy_t (since event is an object) and wires it to the original thing +
// increfs the original and new_thing_values the cow proxy
sentry_value_t cow_event = sentry__value_cow_snapshot(event);
before_send_hook(cow_event);

// inside the hook

// now get_by_key must materialize the direct children of the event
// meaning we must create a new object or a list and COW snapshot each child and 
// copy the proxies over to the new container and return the COW proxy of the user value
sentry_value_t user = sentry_value_get_by_key(event, "user");

// set_by_key now materializes the direct children of the user, allowing us 
// to sink in the new string into the location, divorced from the user and email in scope.
sentry_value_set_by_key(user, "email", sentry_value_new_string("redacted"));

This means the cost model shifted from a full recursive deep clone for every hook invocation to a 1-level clone on first access inside the hook. Users who don't inspect the event pay nothing. Users who read still have to pay the proxy clones on first access, but nothing more. Users who mutate also pay for that, as well as for whatever they add. In the worst case, when someone accesses only deeply nested values, they lazily pay the price of a deep clone.

In addition to the new thing-type, you only need a materialize function and a cow resolver. The only functions that must be changed (they need to materialize and resolve) are the getters and setters of the mutable value types and the thing functions (value_as* and thing_free).

Note: We could make copying the children during materialization lazy (making the first access even cheaper), but that would actually add quite a bit of complexity downstream (because every function that counts or iterates must now be aware of incomplete objects and lists). I am not sure if that would be worth it as a first step.

Does that make sense?