fix: user-feedback comment guard (#1020)

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**:
 

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

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 <stdio.h>
 #include <string.h>
@@ -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);
 

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;
 }
 

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.
  */

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

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;
     }

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

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);
+}

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)