-
Notifications
You must be signed in to change notification settings - Fork 733
Async safe number to string conversion #667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
* Async-safe number-to-string conversion * Changes requested in code review --------- Co-authored-by: Robert <[email protected]>
What about places like KSCrash/Sources/KSCrashRecording/KSCrashReportC.c Lines 923 to 941 in 19c1fb6
writeBasicRegisters can also be called from a signal handler and it still uses snprintf .
|
memcpy(nameBuffer, "stack@0x", 8); | ||
char *addressStart = nameBuffer + 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use sizeeof
to make it more error-proof?
memcpy(nameBuffer, "stack@0x", 8); | |
char *addressStart = nameBuffer + 8; | |
static const char prefix[] = "stack@0x"; | |
memcpy(nameBuffer, prefix, sizeof(prefix) - 1); | |
char *addressStart = nameBuffer + sizeof(prefix) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces async-safe number to string conversion functions to replace unsafe system calls like sprintf
in crash reporting contexts. The implementation adds custom hex and UUID string conversion functions that avoid potential deadlocks during crash handling.
- Adds
KSStringConversion
module with async-safeuint64_to_hex
anduuid_to_string
functions - Replaces
sprintf
call in crash reporting with the new async-safe hex conversion - Includes comprehensive unit tests and integration tests for the new functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
Sources/KSCrashRecordingCore/include/KSStringConversion.h | Header defining async-safe string conversion functions |
Sources/KSCrashRecordingCore/KSStringConversion.c | Implementation of hex and UUID string conversion utilities |
Sources/KSCrashRecording/KSCrashReportC.c | Updates crash reporting to use async-safe hex conversion instead of sprintf |
Tests/KSCrashRecordingCoreTests/KSStringConversion_Tests.m | Unit tests for the new string conversion functions |
Sources/KSCrashCore/include/KSCrashNamespace.h | Namespace registration for new functions |
Samples/Tests/IntegrationTests.swift | Integration test validating memory introspection with new formatting |
Samples/Tests/Core/PartialCrashReport.swift | Test model updates to support notable addresses validation |
Samples/Common/Sources/IntegrationTestsHelper/InstallConfig.swift | Test configuration support for memory introspection |
if (segmentIndex != 4) { | ||
memcpy(currentDst++, "-", 1); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function kssc_uuid_to_string
does not null-terminate the output string. The destination buffer should have a null terminator added after the UUID string is written.
} | |
} | |
*currentDst = '\0'; |
Copilot uses AI. Check for mistakes.
sprintf(nameBuffer, "stack@%p", (void *)address); | ||
memcpy(nameBuffer, "stack@0x", 8); | ||
char *addressStart = nameBuffer + 8; | ||
kssc_uint64_to_hex((uintptr_t)address, addressStart, 1, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output from kssc_uint64_to_hex
is not null-terminated in the destination buffer. Since nameBuffer
is used as a string, it needs proper null termination after the hex conversion.
kssc_uint64_to_hex((uintptr_t)address, addressStart, 1, false); | |
int hexLen = kssc_uint64_to_hex((uintptr_t)address, addressStart, 1, false); | |
nameBuffer[8 + hexLen] = '\0'; |
Copilot uses AI. Check for mistakes.
currentDst += 2; | ||
} | ||
if (segmentIndex != 4) { | ||
memcpy(currentDst++, "-", 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using memcpy
to copy a single character is unnecessary. A simple assignment *currentDst++ = '-';
would be more efficient and clearer.
memcpy(currentDst++, "-", 1); | |
*currentDst++ = '-'; |
Copilot uses AI. Check for mistakes.
No description provided.