diff --git a/CHANGELOG.md b/CHANGELOG.md index bf6f54a46..42d162510 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Further clean up of the exported dependency configuration. ([#1013](https://github.com/getsentry/sentry-native/pull/1013), [crashpad#106](https://github.com/getsentry/crashpad/pull/106)) - Clean-up scope flushing synchronization in crashpad-backend. ([#1019](https://github.com/getsentry/sentry-native/pull/1019), [crashpad#109](https://github.com/getsentry/crashpad/pull/109)) +- Rectify user-feedback comment parameter guard. ([#1020](https://github.com/getsentry/sentry-native/pull/1020)) **Internal**: diff --git a/src/sentry_core.c b/src/sentry_core.c index 55241aa75..8b35d0469 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -1002,10 +1002,9 @@ sentry_span_t * sentry_transaction_start_child(sentry_transaction_t *opaque_parent, const char *operation, const char *description) { - const size_t operation_len = operation ? strlen(operation) : 0; - const size_t description_len = description ? strlen(description) : 0; - return sentry_transaction_start_child_n( - opaque_parent, operation, operation_len, description, description_len); + return sentry_transaction_start_child_n(opaque_parent, operation, + sentry__guarded_strlen(operation), description, + sentry__guarded_strlen(description)); } sentry_span_t * @@ -1040,10 +1039,9 @@ sentry_span_t * sentry_span_start_child(sentry_span_t *opaque_parent, const char *operation, const char *description) { - size_t operation_len = operation ? strlen(operation) : 0; - size_t description_len = description ? strlen(description) : 0; - return sentry_span_start_child_n( - opaque_parent, operation, operation_len, description, description_len); + return sentry_span_start_child_n(opaque_parent, operation, + sentry__guarded_strlen(operation), description, + sentry__guarded_strlen(description)); } void diff --git a/src/sentry_logger.c b/src/sentry_logger.c index 6a0f4703e..0dc8f05d5 100644 --- a/src/sentry_logger.c +++ b/src/sentry_logger.c @@ -1,6 +1,7 @@ #include "sentry_logger.h" #include "sentry_core.h" #include "sentry_options.h" +#include "sentry_string.h" #include #include @@ -52,7 +53,8 @@ sentry__logger_defaultlogger( const char *prefix = "[sentry] "; const char *priority = sentry__logger_describe(level); - size_t len = strlen(prefix) + strlen(priority) + strlen(message) + 2; + size_t len = strlen(prefix) + strlen(priority) + + sentry__guarded_strlen(message) + 2; char *format = sentry_malloc(len); snprintf(format, len, "%s%s%s\n", prefix, priority, message); diff --git a/src/sentry_slice.c b/src/sentry_slice.c index 0f68311b2..43880fb8b 100644 --- a/src/sentry_slice.c +++ b/src/sentry_slice.c @@ -8,7 +8,7 @@ sentry__slice_from_str(const char *str) { sentry_slice_t rv; rv.ptr = str; - rv.len = str ? strlen(str) : 0; + rv.len = sentry__guarded_strlen(str); return rv; } diff --git a/src/sentry_string.h b/src/sentry_string.h index 95e1e4f61..ee933d4da 100644 --- a/src/sentry_string.h +++ b/src/sentry_string.h @@ -173,6 +173,15 @@ sentry__string_eq(const char *a, const char *b) return strcmp(a, b) == 0; } +/** + * Guards strlen() against NULL pointers + */ +static inline size_t +sentry__guarded_strlen(const char *s) +{ + return s ? strlen(s) : 0; +} + /** * Converts an int64_t into a string. */ diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index 5f3ae7c69..4d6195a0d 100644 --- a/src/sentry_tracing.c +++ b/src/sentry_tracing.c @@ -75,11 +75,8 @@ sentry_transaction_context_new_n(const char *name, size_t name_len, sentry_transaction_context_t * sentry_transaction_context_new(const char *name, const char *operation) { - size_t name_len = name ? strlen(name) : 0; - size_t operation_len = operation ? strlen(operation) : 0; - - return sentry_transaction_context_new_n( - name, name_len, operation, operation_len); + return sentry_transaction_context_new_n(name, sentry__guarded_strlen(name), + operation, sentry__guarded_strlen(operation)); } void @@ -212,11 +209,8 @@ void sentry_transaction_context_update_from_header( sentry_transaction_context_t *tx_cxt, const char *key, const char *value) { - size_t key_len = key ? strlen(key) : 0; - size_t value_len = value ? strlen(value) : 0; - - sentry_transaction_context_update_from_header_n( - tx_cxt, key, key_len, value, value_len); + sentry_transaction_context_update_from_header_n(tx_cxt, key, + sentry__guarded_strlen(key), value, sentry__guarded_strlen(value)); } sentry_transaction_t * @@ -338,11 +332,8 @@ sentry_value_t sentry__value_span_new(size_t max_spans, sentry_value_t parent, const char *operation, const char *description) { - const size_t operation_len = operation ? strlen(operation) : 0; - const size_t description_len = description ? strlen(description) : 0; return sentry__value_span_new_n(max_spans, parent, - (sentry_slice_t) { operation, operation_len }, - (sentry_slice_t) { description, description_len }); + sentry__slice_from_str(operation), sentry__slice_from_str(description)); } sentry_value_t @@ -417,10 +408,7 @@ set_tag_n(sentry_value_t item, sentry_slice_t tag, sentry_slice_t value) static void set_tag(sentry_value_t item, const char *tag, const char *value) { - const size_t tag_len = tag ? strlen(tag) : 0; - const size_t value_len = value ? strlen(value) : 0; - set_tag_n(item, (sentry_slice_t) { tag, tag_len }, - (sentry_slice_t) { value, value_len }); + set_tag_n(item, sentry__slice_from_str(tag), sentry__slice_from_str(value)); } void diff --git a/src/sentry_utils.c b/src/sentry_utils.c index f2980b728..62c36758f 100644 --- a/src/sentry_utils.c +++ b/src/sentry_utils.c @@ -418,7 +418,7 @@ sentry__usec_time_to_iso8601(uint64_t time) uint64_t sentry__iso8601_to_usec(const char *iso) { - size_t len = strlen(iso); + size_t len = sentry__guarded_strlen(iso); if (len != 20 && len != 27) { return 0; } diff --git a/src/sentry_value.c b/src/sentry_value.c index e2315914b..ffad22ffe 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -742,8 +742,7 @@ sentry_value_get_by_key_n(sentry_value_t value, const char *k, size_t k_len) sentry_value_t sentry_value_get_by_key(sentry_value_t value, const char *k) { - const size_t k_len = k ? strlen(k) : 0; - return sentry_value_get_by_key_n(value, k, k_len); + return sentry_value_get_by_key_n(value, k, sentry__guarded_strlen(k)); } sentry_value_t @@ -1151,10 +1150,8 @@ sentry_value_t sentry_value_new_message_event( sentry_level_t level, const char *logger, const char *text) { - size_t logger_len = logger ? strlen(logger) : 0; - size_t text_len = text ? strlen(text) : 0; - return sentry_value_new_message_event_n( - level, logger, logger_len, text, text_len); + return sentry_value_new_message_event_n(level, logger, + sentry__guarded_strlen(logger), text, sentry__guarded_strlen(text)); } static void @@ -1186,9 +1183,8 @@ sentry_value_new_breadcrumb_n( sentry_value_t sentry_value_new_breadcrumb(const char *type, const char *message) { - const size_t type_len = type ? strlen(type) : 0; - const size_t message_len = message ? strlen(message) : 0; - return sentry_value_new_breadcrumb_n(type, type_len, message, message_len); + return sentry_value_new_breadcrumb_n(type, sentry__guarded_strlen(type), + message, sentry__guarded_strlen(message)); } sentry_value_t @@ -1206,9 +1202,8 @@ sentry_value_new_exception_n( sentry_value_t sentry_value_new_exception(const char *type, const char *value) { - const size_t type_len = type ? strlen(type) : 0; - const size_t value_len = value ? strlen(value) : 0; - return sentry_value_new_exception_n(type, type_len, value, value_len); + return sentry_value_new_exception_n(type, sentry__guarded_strlen(type), + value, sentry__guarded_strlen(value)); } sentry_value_t @@ -1236,8 +1231,7 @@ sentry_value_new_thread_n(uint64_t id, const char *name, size_t name_len) sentry_value_t sentry_value_new_thread(uint64_t id, const char *name) { - const size_t name_len = name ? strlen(name) : 0; - return sentry_value_new_thread_n(id, name, name_len); + return sentry_value_new_thread_n(id, name, sentry__guarded_strlen(name)); } sentry_value_t @@ -1269,11 +1263,9 @@ sentry_value_t sentry_value_new_user_feedback(const sentry_uuid_t *uuid, const char *name, const char *email, const char *comments) { - size_t name_len = name ? strlen(name) : 0; - size_t email_len = email ? strlen(email) : 0; - size_t comments_len = email ? strlen(comments) : 0; - return sentry_value_new_user_feedback_n( - uuid, name, name_len, email, email_len, comments, comments_len); + return sentry_value_new_user_feedback_n(uuid, name, + sentry__guarded_strlen(name), email, sentry__guarded_strlen(email), + comments, sentry__guarded_strlen(comments)); } sentry_value_t diff --git a/tests/unit/test_value.c b/tests/unit/test_value.c index d1012602b..40257b182 100644 --- a/tests/unit/test_value.c +++ b/tests/unit/test_value.c @@ -793,3 +793,67 @@ SENTRY_TEST(user_feedback_is_valid) sentry_value_decref(user_feedback); } + +SENTRY_TEST(user_feedback_with_null_args) +{ + sentry_uuid_t event_id + = sentry_uuid_from_string("c993afb6-b4ac-48a6-b61b-2558e601d65d"); + sentry_value_t user_feedback + = sentry_value_new_user_feedback(&event_id, NULL, NULL, NULL); + + TEST_CHECK(!sentry_value_is_null(user_feedback)); + TEST_CHECK( + sentry_value_is_null(sentry_value_get_by_key(user_feedback, "name"))); + TEST_CHECK( + sentry_value_is_null(sentry_value_get_by_key(user_feedback, "email"))); + TEST_CHECK(sentry_value_is_null( + sentry_value_get_by_key(user_feedback, "comments"))); + + sentry_value_decref(user_feedback); + + user_feedback = sentry_value_new_user_feedback( + &event_id, NULL, "some-email", "some-comment"); + + TEST_CHECK(!sentry_value_is_null(user_feedback)); + TEST_CHECK( + sentry_value_is_null(sentry_value_get_by_key(user_feedback, "name"))); + TEST_CHECK_STRING_EQUAL( + sentry_value_as_string(sentry_value_get_by_key(user_feedback, "email")), + "some-email"); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(sentry_value_get_by_key( + user_feedback, "comments")), + "some-comment"); + + sentry_value_decref(user_feedback); + + user_feedback = sentry_value_new_user_feedback( + &event_id, "some-name", NULL, "some-comment"); + + TEST_CHECK(!sentry_value_is_null(user_feedback)); + TEST_CHECK_STRING_EQUAL( + sentry_value_as_string(sentry_value_get_by_key(user_feedback, "name")), + "some-name"); + TEST_CHECK( + sentry_value_is_null(sentry_value_get_by_key(user_feedback, "email"))); + TEST_CHECK_STRING_EQUAL(sentry_value_as_string(sentry_value_get_by_key( + user_feedback, "comments")), + "some-comment"); + + sentry_value_decref(user_feedback); + + user_feedback = sentry_value_new_user_feedback( + &event_id, "some-name", "some-email", NULL); + + TEST_CHECK(!sentry_value_is_null(user_feedback)); + + TEST_CHECK_STRING_EQUAL( + sentry_value_as_string(sentry_value_get_by_key(user_feedback, "name")), + "some-name"); + TEST_CHECK_STRING_EQUAL( + sentry_value_as_string(sentry_value_get_by_key(user_feedback, "email")), + "some-email"); + TEST_CHECK(sentry_value_is_null( + sentry_value_get_by_key(user_feedback, "comments"))); + + sentry_value_decref(user_feedback); +} diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index d7f9fae29..0f0db85db 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -109,6 +109,7 @@ XX(url_parsing_complete) XX(url_parsing_invalid) XX(url_parsing_partial) XX(user_feedback_is_valid) +XX(user_feedback_with_null_args) XX(uuid_api) XX(uuid_v4) XX(value_bool)