Skip to content

Commit d00bcc9

Browse files
committed
Fix memory safety issues and buffer handling bugs
- Fix memmove bug in KSFileUtils.c fillReadBuffer that used wrong byte count - Add explicit (size_t) casts to prevent signed/unsigned conversion warnings - Replace sprintf with snprintf to prevent potential buffer overflows - Update test bundle accessor to use modern SPM resource bundle API
1 parent 0780f8d commit d00bcc9

File tree

12 files changed

+28
-27
lines changed

12 files changed

+28
-27
lines changed

Package.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,10 +470,10 @@ let package = Package(
470470
.copy("Resources/PrivacyInfo.xcprivacy")
471471
],
472472
cSettings: [
473-
.unsafeFlags(warningFlags),
473+
.unsafeFlags(warningFlags)
474474
],
475475
cxxSettings: [
476-
.unsafeFlags(warningFlags),
476+
.unsafeFlags(warningFlags)
477477
]
478478
),
479479
.testTarget(

Sources/KSCrashRecording/KSCrashReportC.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,7 @@ static void writeNotableStackContents(const KSCrashReportWriter *const writer,
907907
char nameBuffer[40];
908908
for (uintptr_t address = lowAddress; address < highAddress; address += sizeof(address)) {
909909
if (ksmem_copySafely((void *)address, &contentsAsPointer, sizeof(contentsAsPointer))) {
910-
sprintf(nameBuffer, "stack@%p", (void *)address);
910+
snprintf(nameBuffer, sizeof(nameBuffer), "stack@%p", (void *)address);
911911
writeMemoryContentsIfNotable(writer, nameBuffer, contentsAsPointer);
912912
}
913913
}

Sources/KSCrashRecording/KSCrashReportFixer.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ static int addJSONData(const char *data, int length, void *userData)
235235
if (length > context->outputBytesLeft) {
236236
return KSJSON_ERROR_DATA_TOO_LONG;
237237
}
238-
memcpy(context->outputPtr, data, length);
238+
memcpy(context->outputPtr, data, (size_t)length);
239239
context->outputPtr += length;
240240
context->outputBytesLeft -= length;
241241

@@ -265,7 +265,7 @@ static char *kscrf_fixupCrashReportWithVersionComponents(const char *crashReport
265265
char *stringBuffer = malloc((unsigned)stringBufferLength);
266266
int crashReportLength = (int)strlen(crashReport);
267267
int fixedReportLength = (int)(crashReportLength * 1.5);
268-
char *fixedReport = malloc((unsigned)fixedReportLength);
268+
char *fixedReport = malloc((unsigned)fixedReportLength + 1); // +1 for null terminator
269269
KSJSONEncodeContext encodeContext;
270270
FixupContext fixupContext = {
271271
.encodeContext = &encodeContext,

Sources/KSCrashRecording/KSCrashReportStoreC.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ static void getCrashReportPathByID(int64_t id, char *pathBuffer, const KSCrashRe
6767
static int64_t getReportIDFromFilename(const char *filename, const KSCrashReportStoreCConfiguration *const config)
6868
{
6969
char scanFormat[100];
70-
sprintf(scanFormat, "%s-report-%%" PRIx64 ".json", config->appName);
70+
snprintf(scanFormat, sizeof(scanFormat), "%s-report-%%" PRIx64 ".json", config->appName);
7171

7272
int64_t reportID = 0;
7373
sscanf(filename, scanFormat, &reportID);

Sources/KSCrashRecording/Monitors/KSCrashMonitor_CPPException.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ static void CPPExceptionTerminate(void)
134134
KSCrash_MonitorContext *crashContext = g_state.callbacks.notify(
135135
thisThread,
136136
(KSCrash_ExceptionHandlingRequirements) {
137-
.asyncSafety = true, .isFatal = true, .shouldRecordAllThreads = true, .shouldWriteReport = true });
137+
.shouldRecordAllThreads = true, .shouldWriteReport = true, .isFatal = true, .asyncSafety = true });
138138
if (crashContext->requirements.shouldExitImmediately) {
139139
goto skip_handling;
140140
}

Sources/KSCrashRecordingCore/KSFileUtils.c

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -148,14 +148,14 @@ static bool deletePathContents(const char *path, bool deleteTopLevelPathAlso)
148148

149149
int bufferLength = KSFU_MAX_PATH_LENGTH;
150150
char *pathBuffer = malloc((unsigned)bufferLength);
151-
snprintf(pathBuffer, bufferLength, "%s/", path);
151+
snprintf(pathBuffer, (size_t)bufferLength, "%s/", path);
152152
char *pathPtr = pathBuffer + strlen(pathBuffer);
153153
int pathRemainingLength = bufferLength - (int)(pathPtr - pathBuffer);
154154

155155
for (int i = 0; i < entryCount; i++) {
156156
char *entry = entries[i];
157157
if (entry != NULL && canDeletePath(entry)) {
158-
strncpy(pathPtr, entry, pathRemainingLength);
158+
strncpy(pathPtr, entry, (size_t)pathRemainingLength);
159159
deletePathContents(pathBuffer, true);
160160
}
161161
}
@@ -421,7 +421,7 @@ bool ksfu_writeBufferedWriter(KSBufferedWriter *writer, const char *restrict con
421421
if (length > writer->bufferLength) {
422422
return ksfu_writeBytesToFD(writer->fd, data, length);
423423
}
424-
memcpy(writer->buffer + writer->position, data, length);
424+
memcpy(writer->buffer + writer->position, data, (size_t)length);
425425
writer->position += length;
426426
return true;
427427
}
@@ -442,8 +442,9 @@ static inline bool isReadBufferEmpty(KSBufferedReader *reader) { return reader->
442442
static bool fillReadBuffer(KSBufferedReader *reader)
443443
{
444444
if (reader->dataStartPos > 0) {
445-
memmove(reader->buffer, reader->buffer + reader->dataStartPos, reader->dataStartPos);
446-
reader->dataEndPos -= reader->dataStartPos;
445+
int remainingData = reader->dataEndPos - reader->dataStartPos;
446+
memmove(reader->buffer, reader->buffer + reader->dataStartPos, (size_t)remainingData);
447+
reader->dataEndPos = remainingData;
447448
reader->dataStartPos = 0;
448449
reader->buffer[reader->dataEndPos] = '\0';
449450
}
@@ -480,7 +481,7 @@ int ksfu_readBufferedReader(KSBufferedReader *reader, char *dstBuffer, int byteC
480481
}
481482
int bytesToCopy = bytesInReader <= bytesRemaining ? bytesInReader : bytesRemaining;
482483
char *pSrc = reader->buffer + reader->dataStartPos;
483-
memcpy(pDst, pSrc, bytesToCopy);
484+
memcpy(pDst, pSrc, (size_t)bytesToCopy);
484485
pDst += bytesToCopy;
485486
reader->dataStartPos += bytesToCopy;
486487
bytesConsumed += bytesToCopy;
@@ -507,7 +508,7 @@ bool ksfu_readBufferedReaderUntilChar(KSBufferedReader *reader, int ch, char *ds
507508
bytesToCopy = bytesToChar;
508509
}
509510
}
510-
memcpy(pDst, pSrc, bytesToCopy);
511+
memcpy(pDst, pSrc, (size_t)bytesToCopy);
511512
pDst += bytesToCopy;
512513
reader->dataStartPos += bytesToCopy;
513514
bytesConsumed += bytesToCopy;

Sources/KSCrashRecordingCore/KSID.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ void ksid_generate(char *destinationBuffer37Bytes)
3131
{
3232
uuid_t uuid;
3333
uuid_generate(uuid);
34-
sprintf(destinationBuffer37Bytes, "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-%02X%02X%02X%02X%02X%02X",
35-
(unsigned)uuid[0], (unsigned)uuid[1], (unsigned)uuid[2], (unsigned)uuid[3], (unsigned)uuid[4],
36-
(unsigned)uuid[5], (unsigned)uuid[6], (unsigned)uuid[7], (unsigned)uuid[8], (unsigned)uuid[9],
37-
(unsigned)uuid[10], (unsigned)uuid[11], (unsigned)uuid[12], (unsigned)uuid[13], (unsigned)uuid[14],
38-
(unsigned)uuid[15]);
34+
snprintf(destinationBuffer37Bytes, 37, "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-%02X%02X%02X%02X%02X%02X",
35+
(unsigned)uuid[0], (unsigned)uuid[1], (unsigned)uuid[2], (unsigned)uuid[3], (unsigned)uuid[4],
36+
(unsigned)uuid[5], (unsigned)uuid[6], (unsigned)uuid[7], (unsigned)uuid[8], (unsigned)uuid[9],
37+
(unsigned)uuid[10], (unsigned)uuid[11], (unsigned)uuid[12], (unsigned)uuid[13], (unsigned)uuid[14],
38+
(unsigned)uuid[15]);
3939
}

Sources/KSCrashRecordingCore/KSJSONCodec.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,7 @@ static int decodeString(KSJSONDecodeContext *context, char *dstBuffer, int dstBu
739739
// If no escape characters were encountered, we can fast copy.
740740
likely_if(fastCopy)
741741
{
742-
memcpy(dstBuffer, src, length);
742+
memcpy(dstBuffer, src, (size_t)length);
743743
dstBuffer[length] = 0;
744744
return KSJSON_OK;
745745
}
@@ -1038,7 +1038,7 @@ static int decodeElement(const char *const name, KSJSONDecodeContext *context)
10381038
KSLOG_DEBUG("Number is too long.");
10391039
return KSJSON_ERROR_DATA_TOO_LONG;
10401040
}
1041-
strncpy(context->stringBuffer, start, len);
1041+
strncpy(context->stringBuffer, start, (size_t)len);
10421042
context->stringBuffer[len] = '\0';
10431043

10441044
sscanf(context->stringBuffer, "%lg", &value);
@@ -1105,7 +1105,7 @@ static void updateDecoder_readFile(struct JSONFromFileContext *context)
11051105
unlikely_if(remainingLength < bufferLength / 2)
11061106
{
11071107
int fillLength = bufferLength - remainingLength;
1108-
memcpy(start, ptr, remainingLength);
1108+
memcpy(start, ptr, (size_t)remainingLength);
11091109
context->decodeContext->bufferPtr = start;
11101110
int bytesRead = (int)read(context->fd, start + remainingLength, (unsigned)fillLength);
11111111
unlikely_if(bytesRead < fillLength)

Sources/KSCrashRecordingCore/KSObjC.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ static int stringPrintf(char *buffer, int bufferLength, const char *fmt, ...)
452452

453453
va_list args;
454454
va_start(args, fmt);
455-
int printLength = vsnprintf(buffer, bufferLength, fmt, args);
455+
int printLength = vsnprintf(buffer, (size_t)bufferLength, fmt, args);
456456
va_end(args);
457457

458458
unlikely_if(printLength < 0)

Sources/KSCrashRecordingCore/KSThread.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ bool ksthread_getQueueName(const KSThread thread, char *const buffer, int bufLen
151151
return false;
152152
}
153153
bufLength = MIN(length, bufLength - 1); // just strlen, without null-terminator
154-
strncpy(buffer, queue_name, bufLength);
154+
strncpy(buffer, queue_name, (size_t)bufLength);
155155
buffer[bufLength] = 0; // terminate string
156156
KSLOG_TRACE("Queue label = %s", buffer);
157157
return true;

0 commit comments

Comments
 (0)