diff --git a/CHANGELOG.md b/CHANGELOG.md index 356b9ede8..431ad1740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +**Fixes**: + +- Use a signal-safe address formatter instead of `snprintf()`. ([#1444](https://github.com/getsentry/sentry-native/pull/1444)) + ## 0.12.1 **Fixes**: diff --git a/src/modulefinder/sentry_modulefinder_apple.c b/src/modulefinder/sentry_modulefinder_apple.c index 26ef5088e..b7c8d7266 100644 --- a/src/modulefinder/sentry_modulefinder_apple.c +++ b/src/modulefinder/sentry_modulefinder_apple.c @@ -106,7 +106,8 @@ remove_image(const struct mach_header *mh, intptr_t UNUSED(vmaddr_slide)) } char ref_addr[32]; - snprintf(ref_addr, sizeof(ref_addr), "0x%llx", (long long)info.dli_fbase); + sentry__addr_to_string( + ref_addr, sizeof(ref_addr), (uint64_t)info.dli_fbase); sentry_value_t new_modules = sentry_value_new_list(); for (size_t i = 0; i < sentry_value_get_length(g_modules); i++) { diff --git a/src/sentry_string.c b/src/sentry_string.c index 5952888dd..76f4ce0be 100644 --- a/src/sentry_string.c +++ b/src/sentry_string.c @@ -7,6 +7,46 @@ # include #endif +// "0x" + 16 nibbles + NUL +#define SENTRY_ADDR_MIN_BUFFER_SIZE 19 +/** + * We collect hex digits into a small stack scratch buffer (in reverse order) + * and then copy them forward. This avoids reverse-writing into the destination + * and keeps the code simple. + */ +bool +sentry__addr_to_string(char *buf, size_t buf_len, uint64_t addr) +{ + static const char hex[] = "0123456789abcdef"; + + if (!buf || buf_len < SENTRY_ADDR_MIN_BUFFER_SIZE) { + return false; + } + + size_t buf_idx = 0; + buf[buf_idx++] = '0'; + buf[buf_idx++] = 'x'; + + // fill a reverse buffer from each nibble + char rev[2 * sizeof(uint64_t)]; + size_t rev_idx = 0; + if (addr == 0) { + rev[rev_idx++] = '0'; + } else { + while (addr && rev_idx < sizeof(rev)) { + rev[rev_idx++] = hex[addr & 0xF]; + addr >>= 4; + } + } + + // read rev into buf from its end + while (rev_idx && buf_idx + 1 < buf_len) { + buf[buf_idx++] = rev[--rev_idx]; + } + buf[buf_idx] = '\0'; + return true; +} + #define INITIAL_BUFFER_SIZE 128 void diff --git a/src/sentry_string.h b/src/sentry_string.h index 710924e7f..74dba6812 100644 --- a/src/sentry_string.h +++ b/src/sentry_string.h @@ -204,6 +204,15 @@ sentry__uint64_to_string(uint64_t val) return sentry__string_clone(buf); } +/** + * Formats an address as "0x" + lowercase hex into a caller-provided buffer. + * This is a replacement for `snprintf` in signal handlers: + * - signal-safe: uses no stdio, malloc, locks, or thread-local state. + * - reentrant: only stack locals; no writable globals. + * Returns 0 on success. + */ +bool sentry__addr_to_string(char *buf, size_t buf_len, uint64_t addr); + #ifdef SENTRY_PLATFORM_WINDOWS /** * Create a utf-8 string from a Wide String. diff --git a/src/sentry_value.c b/src/sentry_value.c index af4c8709b..feccef286 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -1191,12 +1191,7 @@ sentry_value_t sentry__value_new_addr(uint64_t addr) { char buf[32]; - size_t written = (size_t)snprintf( - buf, sizeof(buf), "0x%llx", (unsigned long long)addr); - if (written >= sizeof(buf)) { - return sentry_value_new_null(); - } - buf[written] = '\0'; + sentry__addr_to_string(buf, sizeof(buf), addr); return sentry_value_new_string(buf); } diff --git a/tests/assertions.py b/tests/assertions.py index 05d3ea320..62e5a2cb0 100644 --- a/tests/assertions.py +++ b/tests/assertions.py @@ -170,6 +170,16 @@ def assert_event_meta( ) +def is_valid_hex(s): + if not s.lower().startswith("0x"): + return False + try: + int(s, 0) + return True + except ValueError: + return False + + def assert_stacktrace( envelope, inside_exception=False, check_size=True, check_package=False ): @@ -181,7 +191,7 @@ def assert_stacktrace( if check_size: assert len(frames) > 0 - assert all(frame["instruction_addr"].startswith("0x") for frame in frames) + assert all(is_valid_hex(frame["instruction_addr"]) for frame in frames) assert any( frame.get("function") is not None and frame.get("package") is not None for frame in frames diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index b6c8dc0fc..9cd240d79 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -44,6 +44,7 @@ add_executable(sentry_test_unit test_scope.c test_session.c test_slice.c + test_string.c test_symbolizer.c test_sync.c test_tracing.c diff --git a/tests/unit/test_string.c b/tests/unit/test_string.c new file mode 100644 index 000000000..317084906 --- /dev/null +++ b/tests/unit/test_string.c @@ -0,0 +1,30 @@ +#include "sentry_random.h" +#include "sentry_string.h" +#include "sentry_testsupport.h" + +void +assert_addr_value_equals_format(uint64_t addr) +{ + char our_buf[32]; + char stdio_buf[32]; + sentry__addr_to_string(our_buf, sizeof(our_buf), addr); + snprintf(stdio_buf, sizeof(stdio_buf), "0x%" PRIx64, addr); + TEST_CHECK_STRING_EQUAL(our_buf, stdio_buf); +} + +SENTRY_TEST(string_address_format) +{ + assert_addr_value_equals_format(0); + assert_addr_value_equals_format(0xf000000000000000); + assert_addr_value_equals_format(0x000000000000000f); + assert_addr_value_equals_format(0x0000ffff0000ffff); + assert_addr_value_equals_format(0x0000ffffffff0000); + assert_addr_value_equals_format(0xf0f0f0f0f0f0f0f0); + assert_addr_value_equals_format(0x0f0f0f0f0f0f0f0f); + uint64_t rnd; + for (int i = 0; i < 1000000; ++i) { + TEST_ASSERT(!sentry__getrandom(&rnd, sizeof(rnd))); + assert_addr_value_equals_format(rnd); + } + assert_addr_value_equals_format(0xffffffffffffffff); +} diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index cdce6a715..1f02949bc 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -162,6 +162,7 @@ XX(span_tagging_n) XX(spans_on_scope) XX(stack_guarantee) XX(stack_guarantee_auto_init) +XX(string_address_format) XX(symbolizer) XX(task_queue) XX(thread_without_name_still_valid)