Skip to content
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.

Sign up for GitHub

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

feat: replace occurences of strncpy to strlcpy #4636

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+581 −99
Open

feat: replace occurences of strncpy to strlcpy #4636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4251e53
feat: rename strncpy to strlcpy; added tests
philprime Dec 13, 2024
3926467
add changelog
philprime Dec 13, 2024
db5da99
add unit test for CPPException description
philprime Dec 16, 2024
4c0eb5e
Merge branch 'main' into philprime/strncpy-replacement
philprime Dec 16, 2024
49be080
add test case for deletePathContents with too long path
philprime Dec 16, 2024
ce9668b
add unit test for oncrash write to file
philprime Dec 16, 2024
9b7cdec
Merge remote-tracking branch 'origin/main' into philprime/strncpy-rep…
philprime Jan 2, 2025
536cc1a
Fix test cases
philprime Jan 2, 2025
e3e0127
fix: testcase of cpp exception error capturing
philprime Jan 2, 2025
ef8f619
fix: fix replacement in SentryCrashJSONCodec
philprime Jan 2, 2025
332142b
Merge remote-tracking branch 'origin/main' into philprime/strncpy-rep…
philprime Jan 2, 2025
b8efac1
fix: changelog
philprime Jan 2, 2025
6c67afd
fix: add comments why strncpy needs to be used for jsoncodec decoding
philprime Jan 2, 2025
7c9a4ae
Merge branch 'main' into philprime/strncpy-replacement
armcknight Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Replace occurences of `strncpy` with `strlcpy` (#4636)

## 8.43.0

### Features
Expand All @@ -10,19 +16,6 @@
### Fixes

- Remove empty session replay tags (#4667)
- - `SentrySdkInfo.packages` should be an array (#4626)
- Use the same SdkInfo for envelope header and event (#4629)

### Improvements

- Improve compiler error message for missing Swift declarations due to APPLICATION_EXTENSION_API_ONLY (#4603)
- Mask screenshots for errors (#4623)
- Slightly speed up serializing scope (#4661)

### Internal

- Remove loading `integrations` names from `event.extra` (#4627)
- Add Hybrid SDKs API to add extra SDK packages (#4637)
Comment on lines -13 to -25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I resolved this conflict incorrectly. Did we want these items from the beta release's changes repeated in the 8.43.0 RC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, there have been a couple of releases since I opened up this PR. I'll fix this when getting ready to merge.


## 8.43.0-beta.1

Expand Down
20 changes: 12 additions & 8 deletions Sentry.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@
A8AFFCD42907E0CA00967CD7 /* SentryRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A8AFFCD32907E0CA00967CD7 /* SentryRequestTests.swift */; };
A8F17B2E2901765900990B25 /* SentryRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = A8F17B2D2901765900990B25 /* SentryRequest.m */; };
A8F17B342902870300990B25 /* SentryHttpStatusCodeRange.m in Sources */ = {isa = PBXBuildFile; fileRef = A8F17B332902870300990B25 /* SentryHttpStatusCodeRange.m */; };
D4F2B5352D0C69D500649E42 /* SentryCrashCTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D4F2B5342D0C69D100649E42 /* SentryCrashCTests.swift */; };
D8019910286B089000C277F0 /* SentryCrashReportSinkTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D801990F286B089000C277F0 /* SentryCrashReportSinkTests.swift */; };
D802994E2BA836EF000F0081 /* SentryOnDemandReplay.swift in Sources */ = {isa = PBXBuildFile; fileRef = D802994D2BA836EF000F0081 /* SentryOnDemandReplay.swift */; };
D80299502BA83A88000F0081 /* SentryPixelBuffer.swift in Sources */ = {isa = PBXBuildFile; fileRef = D802994F2BA83A88000F0081 /* SentryPixelBuffer.swift */; };
Expand Down Expand Up @@ -835,8 +836,6 @@
D84DAD5A2B1742C1003CF120 /* SentryTestUtilsDynamic.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = D84DAD4D2B17428D003CF120 /* SentryTestUtilsDynamic.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; };
D84F833D2A1CC401005828E0 /* SentrySwiftAsyncIntegration.h in Headers */ = {isa = PBXBuildFile; fileRef = D84F833B2A1CC401005828E0 /* SentrySwiftAsyncIntegration.h */; };
D84F833E2A1CC401005828E0 /* SentrySwiftAsyncIntegration.m in Sources */ = {isa = PBXBuildFile; fileRef = D84F833C2A1CC401005828E0 /* SentrySwiftAsyncIntegration.m */; };
D851527F2C9971020070F669 /* SentryStringUtils.h in Headers */ = {isa = PBXBuildFile; fileRef = D851527E2C9971020070F669 /* SentryStringUtils.h */; };
D85152832C997A280070F669 /* SentryStringUtils.swift in Sources */ = {isa = PBXBuildFile; fileRef = D85152822C997A1F0070F669 /* SentryStringUtils.swift */; };
D85153002CA2B5F60070F669 /* SentrySwiftUI.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = D8199DAA29376E9B0074249E /* SentrySwiftUI.framework */; };
D85153012CA2B5F60070F669 /* SentrySwiftUI.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = D8199DAA29376E9B0074249E /* SentrySwiftUI.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; };
D851530C2CA2B7B00070F669 /* SentryRedactModifierTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D851530B2CA2B7A30070F669 /* SentryRedactModifierTests.swift */; };
Expand Down Expand Up @@ -1868,6 +1867,7 @@
A8AFFCD32907E0CA00967CD7 /* SentryRequestTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryRequestTests.swift; sourceTree = "<group>"; };
A8F17B2D2901765900990B25 /* SentryRequest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryRequest.m; sourceTree = "<group>"; };
A8F17B332902870300990B25 /* SentryHttpStatusCodeRange.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryHttpStatusCodeRange.m; sourceTree = "<group>"; };
D4F2B5342D0C69D100649E42 /* SentryCrashCTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCrashCTests.swift; sourceTree = "<group>"; };
D800942628F82F3A005D3943 /* SwiftDescriptor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SwiftDescriptor.swift; sourceTree = "<group>"; };
D801990F286B089000C277F0 /* SentryCrashReportSinkTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCrashReportSinkTests.swift; sourceTree = "<group>"; };
D802994D2BA836EF000F0081 /* SentryOnDemandReplay.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryOnDemandReplay.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1921,8 +1921,6 @@
D84F833B2A1CC401005828E0 /* SentrySwiftAsyncIntegration.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentrySwiftAsyncIntegration.h; path = include/SentrySwiftAsyncIntegration.h; sourceTree = "<group>"; };
D84F833C2A1CC401005828E0 /* SentrySwiftAsyncIntegration.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentrySwiftAsyncIntegration.m; sourceTree = "<group>"; };
D8511F722BAC8F750015E6FD /* Sentry.modulemap */ = {isa = PBXFileReference; lastKnownFileType = "sourcecode.module-map"; path = Sentry.modulemap; sourceTree = "<group>"; };
D851527E2C9971020070F669 /* SentryStringUtils.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SentryStringUtils.h; sourceTree = "<group>"; };
D85152822C997A1F0070F669 /* SentryStringUtils.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryStringUtils.swift; sourceTree = "<group>"; };
D851530B2CA2B7A30070F669 /* SentryRedactModifierTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryRedactModifierTests.swift; sourceTree = "<group>"; };
D85596F1280580F10041FF8B /* SentryScreenshotIntegration.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryScreenshotIntegration.m; sourceTree = "<group>"; };
D855AD61286ED6A4002573E1 /* SentryCrashTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryCrashTests.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2523,6 +2521,7 @@
63AA75931EB8AEDB00D153DE /* SentryTests */ = {
isa = PBXGroup;
children = (
D4F2B5332D0C69CC00649E42 /* Recording */,
62872B602BA1B84400A4FA7D /* Swift */,
7B3878E92490D90400EBDEA2 /* SentryClient+TestInit.h */,
D8BC83BA2AFCF08C00A662B7 /* SentryUIApplication+Private.h */,
Expand Down Expand Up @@ -2671,7 +2670,6 @@
63FE6FC120DA4C1000CDBAE8 /* SentryCrashVarArgs.h */,
63FE6FBF20DA4C1000CDBAE8 /* SentryDictionaryDeepSearch.h */,
63FE6FBD20DA4C1000CDBAE8 /* SentryDictionaryDeepSearch.m */,
D851527E2C9971020070F669 /* SentryStringUtils.h */,
);
path = Tools;
sourceTree = "<group>";
Expand Down Expand Up @@ -2853,7 +2851,6 @@
0ADC33EF28D9BE690078D980 /* TestSentryUIDeviceWrapper.swift */,
7B984A9E28E572AF001F4BEE /* CrashReport.swift */,
7BF69E062987D1FE002EBCA4 /* SentryCrashDoctorTests.swift */,
D85152822C997A1F0070F669 /* SentryStringUtils.swift */,
);
path = SentryCrash;
sourceTree = "<group>";
Expand Down Expand Up @@ -3634,6 +3631,14 @@
name = Transaction;
sourceTree = "<group>";
};
D4F2B5332D0C69CC00649E42 /* Recording */ = {
isa = PBXGroup;
children = (
D4F2B5342D0C69D100649E42 /* SentryCrashCTests.swift */,
);
path = Recording;
sourceTree = "<group>";
};
D800942328F82E8D005D3943 /* Swift */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -4219,7 +4224,6 @@
7B0DC72F288698F70039995F /* NSMutableDictionary+Sentry.h in Headers */,
63FE713920DA4C1100CDBAE8 /* SentryCrashMach.h in Headers */,
63EED6BE2237923600E02400 /* SentryOptions.h in Headers */,
D851527F2C9971020070F669 /* SentryStringUtils.h in Headers */,
7BD86EC5264A63F6005439DB /* SentrySysctl.h in Headers */,
63BE85701ECEC6DE00DC44F5 /* SentryDateUtils.h in Headers */,
63FE709520DA4C1000CDBAE8 /* SentryCrashReportFilterBasic.h in Headers */,
Expand Down Expand Up @@ -4964,6 +4968,7 @@
7B2A70DF27D60904008B0D15 /* SentryTestThreadWrapper.swift in Sources */,
62F4DDA12C04CB9700588890 /* SentryBaggageSerializationTests.swift in Sources */,
7BE912AF272166DD00E49E62 /* SentryNoOpSpanTests.swift in Sources */,
D4F2B5352D0C69D500649E42 /* SentryCrashCTests.swift in Sources */,
7B56D73524616E5600B842DA /* SentryConcurrentRateLimitsDictionaryTests.swift in Sources */,
7B7D8730248648AD00D2ECFF /* SentryStacktraceBuilderTests.swift in Sources */,
62E081AB29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift in Sources */,
Expand All @@ -4981,7 +4986,6 @@
7BA61CCF247EB59500C130A8 /* SentryCrashUUIDConversionTests.swift in Sources */,
7BBD188D2448453600427C76 /* SentryHttpDateParserTests.swift in Sources */,
7B72D23A28D074BC0014798A /* TestExtensions.swift in Sources */,
D85152832C997A280070F669 /* SentryStringUtils.swift in Sources */,
7BBD18BB24530D2600427C76 /* SentryFileManagerTests.swift in Sources */,
63FE722020DA66EC00CDBAE8 /* SentryCrashObjC_Tests.m in Sources */,
7B58816727FC5D790098B121 /* SentryDiscardReasonMapperTests.swift in Sources */,
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/SentryAsyncSafeLog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ sentry_asyncLogSetFileName(const char *filename, bool overwrite)
fd = open(filename, openMask, 0644);
unlikely_if(fd < 0) { return 1; }
if (filename != g_logFilename) {
strncpy(g_logFilename, filename, sizeof(g_logFilename));
strlcpy(g_logFilename, filename, sizeof(g_logFilename));
}
}

Expand Down
3 changes: 1 addition & 2 deletions Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@

#include "SentryAsyncSafeLog.h"

#include "SentryStringUtils.h"
#include <cxxabi.h>
#include <dlfcn.h>
#include <exception>
Expand Down Expand Up @@ -161,7 +160,7 @@ CPPExceptionTerminate(void)
try {
throw;
} catch (std::exception &exc) {
strncpy_safe(descriptionBuff, exc.what(), sizeof(descriptionBuff));
strlcpy(descriptionBuff, exc.what(), sizeof(descriptionBuff));
}
#define CATCH_VALUE(TYPE, PRINTFTYPE) \
catch (TYPE value) \
Expand Down
2 changes: 1 addition & 1 deletion Sources/SentryCrash/Recording/SentryCrashC.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ onCrash(struct SentryCrash_MonitorContext *monitorContext)
} else {
char crashReportFilePath[SentryCrashFU_MAX_PATH_LENGTH];
sentrycrashcrs_getNextCrashReportPath(crashReportFilePath);
strncpy(g_lastCrashReportFilePath, crashReportFilePath, sizeof(g_lastCrashReportFilePath));
strlcpy(g_lastCrashReportFilePath, crashReportFilePath, sizeof(g_lastCrashReportFilePath));
sentrycrashreport_writeStandardReport(monitorContext, crashReportFilePath);
sentrySessionReplaySync_writeInfo();
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/SentryCrash/Recording/SentryCrashReport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,8 +1474,8 @@ sentrycrashreport_writeRecrashReport(
char writeBuffer[1024];
SentryCrashBufferedWriter bufferedWriter;
static char tempPath[SentryCrashFU_MAX_PATH_LENGTH];
strncpy(tempPath, path, sizeof(tempPath) - 10);
strncpy(tempPath + strlen(tempPath) - 5, ".old", 5);
strlcpy(tempPath, path, sizeof(tempPath) - 10);
strlcpy(tempPath + strlen(tempPath) - 5, ".old", 5);
SENTRY_ASYNC_SAFE_LOG_INFO("Writing recrash report to %s", path);

if (rename(path, tempPath) < 0) {
Expand Down
2 changes: 1 addition & 1 deletion Sources/SentryCrash/Recording/SentryCrashReportFixer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ increaseDepth(FixupContext *context, const char *name)
if (name == NULL) {
*context->objectPath[context->currentDepth] = '\0';
} else {
strncpy(context->objectPath[context->currentDepth], name,
strlcpy(context->objectPath[context->currentDepth], name,
sizeof(context->objectPath[context->currentDepth]));
}
context->currentDepth++;
Expand Down
2 changes: 1 addition & 1 deletion Sources/SentryCrash/Recording/Tools/SentryCrashFileUtils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ deletePathContents(const char *path, bool deleteTopLevelPathAlso)
for (int i = 0; i < entryCount; i++) {
char *entry = entries[i];
if (entry != NULL && canDeletePath(entry)) {
strncpy(pathPtr, entry, pathRemainingLength);
strlcpy(pathPtr, entry, pathRemainingLength);
deletePathContents(pathBuffer, true);
}
}
Expand Down
9 changes: 9 additions & 0 deletions Sources/SentryCrash/Recording/Tools/SentryCrashJSONCodec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,9 +1256,18 @@ decodeElement(const char *const name, SentryCrashJSONDecodeContext *context)
SENTRY_ASYNC_SAFE_LOG_DEBUG("Number is too long.");
return SentryCrashJSON_ERROR_DATA_TOO_LONG;
}
// Must use strncpy instead of strlcpy, because of the following reason:
//
// Also note that strlcpy() and strlcat() only operate on true ''C'' strings.
// This means that for strlcpy() src must be NUL-terminated and for strlcat()
// both src and dst must be NUL-terminated.
//
// Source: https://linux.die.net/man/3/strlcpy
strncpy(context->stringBuffer, start, len);
Comment on lines +1261 to 1266
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this context here.

This concerns me a little bit since context->stringBuffer is a char * and can point to different static char arrays of varying length. If we don't trust other areas of our code to get strncpy right, we shouldn't trust operations on a non NULL terminated char * either.

Worth noting that while this is from the original KSCrash implementation that still is like this, the JSON codec was completely rewritten in BugSnag and no longer uses a char * buffer, it all uses a static char array of length 512. They do ultimately use memcpy, which isn't even as safe as strncpy because it can leave uninitialized data in the remainder of the buffer if src contains a NULL terminator before length 😅 Also checked Crashlytics, and they use strncpy and memcpy.

This doesn't need to be handled here now, but we should make a mental note of this: we may want to revisit the general design of the JSON encoder for crash reports one day, depending on how much we care about memory safety.

Also I'd omit the doc portion mentioning strlcat since that's not what's being used

Suggested change
// Also note that strlcpy() and strlcat() only operate on true ''C'' strings.
// This means that for strlcpy() src must be NUL-terminated and for strlcat()
// both src and dst must be NUL-terminated.
//
// Source: https://linux.die.net/man/3/strlcpy
strncpy(context->stringBuffer, start, len);
// Also note that strlcpy() and strlcat() only operate on true ''C'' strings.
// This means that for strlcpy() src must be NUL-terminated...
//
// Source: https://linux.die.net/man/3/strlcpy
strncpy(context->stringBuffer, start, len);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During testing I went ahead an replaced it with strlcpy first, without changing any of the unit tests. It kept crashing with EXC_BAD_ACCESS, even though strncpy and even memcpy did not. I also considered to replace the existing implementation with memcpy but decided to stick with it, simply to not change a running system without exact reason.

Thanks for the cleanup of strlcat, I left it in there so that it's actually a quote, but I also see the missing value of additional text.

context->stringBuffer[len] = '\0';

// Parses a floating point number from stringBuffer into value using %lg format
// %lg uses shortest decimal representation and removes trailing zeros
sscanf(context->stringBuffer, "%lg", &value);

value *= sign;
Expand Down
29 changes: 0 additions & 29 deletions Sources/SentryCrash/Reporting/Filters/Tools/SentryStringUtils.h
View file
Open in desktop

This file was deleted.

Loading
Loading