Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sentry_database.c
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ sentry__process_old_runs(const sentry_options_t *options, uint64_t last_crash)
sentry__envelope_add_session(session_envelope, session);

sentry__session_free(session);
if ((++session_num) >= SENTRY_MAX_ENVELOPE_ITEMS) {
if ((++session_num) >= SENTRY_MAX_ENVELOPE_SESSIONS) {
sentry__capture_envelope(
options->transport, session_envelope);
session_envelope = NULL;
Expand Down
110 changes: 76 additions & 34 deletions src/sentry_envelope.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ struct sentry_envelope_item_s {
sentry_value_t event;
char *payload;
size_t payload_len;
sentry_envelope_item_t *next;
};

struct sentry_envelope_s {
bool is_raw;
union {
struct {
sentry_value_t headers;
sentry_envelope_item_t items[SENTRY_MAX_ENVELOPE_ITEMS];
sentry_envelope_item_t *first_item;
sentry_envelope_item_t *last_item;
size_t item_count;
} items;
struct {
Expand All @@ -41,21 +43,34 @@ envelope_add_item(sentry_envelope_t *envelope)
if (envelope->is_raw) {
return NULL;
}
if (envelope->contents.items.item_count >= SENTRY_MAX_ENVELOPE_ITEMS) {
return NULL;
}

// TODO: Envelopes may have at most one event item or one transaction item,
// and not one of both. Some checking should be done here or in
// `sentry__envelope_add_[transaction|event]` to ensure this can't happen.

sentry_envelope_item_t *rv
= &envelope->contents.items
.items[envelope->contents.items.item_count++];
rv->headers = sentry_value_new_object();
rv->event = sentry_value_new_null();
rv->payload = NULL;
rv->payload_len = 0;
return rv;
// Allocate new item
sentry_envelope_item_t *item = SENTRY_MAKE(sentry_envelope_item_t);
if (!item) {
return NULL;
}

// Initialize item
item->headers = sentry_value_new_object();
item->event = sentry_value_new_null();
item->payload = NULL;
Comment on lines +56 to +60
Copy link

Choose a reason for hiding this comment

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

In envelope_add_item(), when item->headers = sentry_value_new_object() fails (returns NULL), the function continues and returns an item with a NULL headers field. The sentry_value_decref() call on NULL headers during cleanup could cause issues. Consider checking if sentry_value_new_object() succeeded before proceeding.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry_envelope.c#L56-L60

Potential issue: In `envelope_add_item()`, when `item->headers =
sentry_value_new_object()` fails (returns NULL), the function continues and returns an
item with a NULL headers field. The `sentry_value_decref()` call on NULL headers during
cleanup could cause issues. Consider checking if `sentry_value_new_object()` succeeded
before proceeding.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2806251

Copy link
Member Author

Choose a reason for hiding this comment

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

if sentry_value_new_object fails, it returns sentry_value_new_null which we can still decref.

item->payload_len = 0;
item->next = NULL;

// Append to linked list
if (envelope->contents.items.last_item) {
envelope->contents.items.last_item->next = item;
} else {
envelope->contents.items.first_item = item;
}
envelope->contents.items.last_item = item;
envelope->contents.items.item_count++;

return item;
}
Comment on lines 43 to 74
Copy link

Choose a reason for hiding this comment

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

The change from a fixed-size array to dynamic linked list removes the hard limit on envelope items. While the SENTRY_MAX_ENVELOPE_SESSIONS limit is used in sentry_database.c for sessions, there is no longer a check in envelope_add_item() to prevent unlimited growth of envelopes in general. This could potentially lead to memory exhaustion if envelopes are created with many items. Consider adding a check or documentation about expected envelope item limits.
Severity: MEDIUM

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry_envelope.c#L43-L74

Potential issue: The change from a fixed-size array to dynamic linked list removes the
hard limit on envelope items. While the `SENTRY_MAX_ENVELOPE_SESSIONS` limit is used in
`sentry_database.c` for sessions, there is no longer a check in `envelope_add_item()` to
prevent unlimited growth of envelopes in general. This could potentially lead to memory
exhaustion if envelopes are created with many items. Consider adding a check or
documentation about expected envelope item limits.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2806251

Copy link
Member Author

Choose a reason for hiding this comment

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

that's the point of this PR :)


static void
Expand Down Expand Up @@ -141,9 +156,16 @@ sentry_envelope_free(sentry_envelope_t *envelope)
return;
}
sentry_value_decref(envelope->contents.items.headers);
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
envelope_item_cleanup(&envelope->contents.items.items[i]);

// Free all items in the linked list
sentry_envelope_item_t *item = envelope->contents.items.first_item;
while (item) {
sentry_envelope_item_t *next = item->next;
envelope_item_cleanup(item);
sentry_free(item);
item = next;
}

sentry_free(envelope);
}

Expand All @@ -166,6 +188,8 @@ sentry__envelope_new(void)
}

rv->is_raw = false;
rv->contents.items.first_item = NULL;
rv->contents.items.last_item = NULL;
rv->contents.items.item_count = 0;
rv->contents.items.headers = sentry_value_new_object();

Expand Down Expand Up @@ -226,12 +250,13 @@ sentry_envelope_get_event(const sentry_envelope_t *envelope)
if (envelope->is_raw) {
return sentry_value_new_null();
}
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {

if (!sentry_value_is_null(envelope->contents.items.items[i].event)
&& !sentry__event_is_transaction(
envelope->contents.items.items[i].event)) {
return envelope->contents.items.items[i].event;
for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
if (!sentry_value_is_null(item->event)
&& !sentry__event_is_transaction(item->event)) {
return item->event;
}
}
return sentry_value_new_null();
Expand All @@ -243,11 +268,13 @@ sentry_envelope_get_transaction(const sentry_envelope_t *envelope)
if (envelope->is_raw) {
return sentry_value_new_null();
}
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
if (!sentry_value_is_null(envelope->contents.items.items[i].event)
&& sentry__event_is_transaction(
envelope->contents.items.items[i].event)) {
return envelope->contents.items.items[i].event;

for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
if (!sentry_value_is_null(item->event)
&& sentry__event_is_transaction(item->event)) {
return item->event;
}
}
return sentry_value_new_null();
Expand Down Expand Up @@ -657,8 +684,9 @@ sentry__envelope_serialize_into_stringbuilder(
SENTRY_DEBUG("serializing envelope into buffer");
sentry__envelope_serialize_headers_into_stringbuilder(envelope, sb);

for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
const sentry_envelope_item_t *item = &envelope->contents.items.items[i];
for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
sentry__envelope_serialize_item_into_stringbuilder(item, sb);
}
}
Expand All @@ -679,8 +707,9 @@ sentry_envelope_serialize_ratelimited(const sentry_envelope_t *envelope,
sentry__envelope_serialize_headers_into_stringbuilder(envelope, &sb);

size_t serialized_items = 0;
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
const sentry_envelope_item_t *item = &envelope->contents.items.items[i];
for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
if (rl) {
int category = envelope_item_get_ratelimiter_category(item);
if (sentry__rate_limiter_is_disabled(rl, category)) {
Expand Down Expand Up @@ -736,9 +765,9 @@ sentry_envelope_write_to_path(
sentry__jsonwriter_write_value(jw, envelope->contents.items.headers);
sentry__jsonwriter_reset(jw);

for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
const sentry_envelope_item_t *item
= &envelope->contents.items.items[i];
for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
const char newline = '\n';
sentry__filewriter_write(fw, &newline, sizeof(char));

Expand Down Expand Up @@ -967,9 +996,22 @@ sentry__envelope_get_item_count(const sentry_envelope_t *envelope)
const sentry_envelope_item_t *
sentry__envelope_get_item(const sentry_envelope_t *envelope, size_t idx)
{
return !envelope->is_raw && idx < envelope->contents.items.item_count
? &envelope->contents.items.items[idx]
: NULL;
if (envelope->is_raw) {
return NULL;
}

// Traverse linked list to find item at index
size_t current_idx = 0;
for (const sentry_envelope_item_t *item
= envelope->contents.items.first_item;
item; item = item->next) {
if (current_idx == idx) {
return item;
}
current_idx++;
}

return NULL;
}

sentry_value_t
Expand Down
3 changes: 2 additions & 1 deletion src/sentry_envelope.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
#include "sentry_session.h"
#include "sentry_string.h"

#define SENTRY_MAX_ENVELOPE_ITEMS 10
// https://develop.sentry.dev/sdk/data-model/envelopes/#size-limits
#define SENTRY_MAX_ENVELOPE_SESSIONS 100

typedef struct sentry_envelope_item_s sentry_envelope_item_t;

Expand Down
6 changes: 3 additions & 3 deletions tests/test_integration_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,8 @@ def test_abnormal_session(cmake, httpserver):
)
db_dir = tmp_path.joinpath(".sentry-native")
db_dir.mkdir(exist_ok=True)
# 15 exceeds the max envelope items
for i in range(15):
# 101 exceeds the max session items
for i in range(101):
run_dir = db_dir.joinpath(f"foo-{i}.run")
run_dir.mkdir()
with open(run_dir.joinpath("session.json"), "w") as session_file:
Expand All @@ -343,7 +343,7 @@ def test_abnormal_session(cmake, httpserver):
for item in itertools.chain(envelope1, envelope2):
if item.headers.get("type") == "session":
session_count += 1
assert session_count == 15
assert session_count == 101

assert_session(envelope1, {"status": "abnormal", "errors": 0, "duration": 10})

Expand Down
Loading