feat: replace occurences of strncpy to strlcpy

[philprime]

📜 Description

Renames all occurences of strncpy with strlcpy as explained in #2783.

💡 Motivation and Context

I looked all the occurences and tried to create additional unit tests where applicable. Some of the cases using strlcpy have larger buffer sizes set, i.e. SentryCrashFU_MAX_PATH_LENGTH is set to 500, but I was not able to file with a longer name because the OS throws NSPOSIXErrorDomain, code 63, indicating that the file system does not support such long files.

Closes #2783

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• I updated the wizard if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

• Validate if additional tests are necessary
• Add unit tests for CPPExceptionTerminate to test if strncpy_safe and strlcpy do the same
• Add unit tests for sentrycrashreport_writeRecrashReport
• Add unit tests for deletePathContents for too long file paths

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 7c9a4ae

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1236.82 ms	1256.59 ms	19.78 ms
Size	22.31 KiB	760.74 KiB	738.43 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
dd4145f	1233.48 ms	1254.35 ms	20.88 ms
6604dbb	1248.35 ms	1256.14 ms	7.79 ms
b4f8dba	1343.92 ms	1362.96 ms	19.04 ms
a0cc9d6	1232.37 ms	1249.55 ms	17.18 ms
3cba0e8	1250.86 ms	1258.39 ms	7.53 ms
fa38a2b	1217.92 ms	1235.52 ms	17.60 ms
728804f	1341.16 ms	1371.27 ms	30.10 ms
bef2003	1248.18 ms	1258.86 ms	10.68 ms
7f14650	1236.00 ms	1255.66 ms	19.66 ms
b9b0f0a	1251.45 ms	1257.86 ms	6.41 ms

App size

Revision	Plain	With Sentry	Diff
dd4145f	21.58 KiB	540.09 KiB	518.51 KiB
6604dbb	22.84 KiB	402.56 KiB	379.72 KiB
b4f8dba	21.58 KiB	614.87 KiB	593.29 KiB
a0cc9d6	21.58 KiB	706.46 KiB	684.88 KiB
3cba0e8	22.84 KiB	403.19 KiB	380.34 KiB
fa38a2b	21.58 KiB	418.70 KiB	397.12 KiB
728804f	22.85 KiB	411.75 KiB	388.91 KiB
bef2003	22.85 KiB	407.73 KiB	384.88 KiB
7f14650	22.84 KiB	402.63 KiB	379.78 KiB
b9b0f0a	20.76 KiB	434.94 KiB	414.18 KiB

Previous results on branch: philprime/strncpy-replacement

Startup times

Revision	Plain	With Sentry	Diff
d747559	1237.35 ms	1253.76 ms	16.41 ms
c1ccd22	1223.02 ms	1244.00 ms	20.98 ms
30fe95a	1230.39 ms	1243.60 ms	13.22 ms
83a0223	1243.98 ms	1257.29 ms	13.31 ms
12db161	1227.21 ms	1242.79 ms	15.58 ms
3ec7df9	1225.36 ms	1247.29 ms	21.93 ms

App size

Revision	Plain	With Sentry	Diff
d747559	22.31 KiB	760.76 KiB	738.44 KiB
c1ccd22	22.31 KiB	758.83 KiB	736.52 KiB
30fe95a	22.31 KiB	757.18 KiB	734.87 KiB
83a0223	22.31 KiB	760.68 KiB	738.36 KiB
12db161	22.31 KiB	757.18 KiB	734.88 KiB
3ec7df9	22.31 KiB	756.53 KiB	734.22 KiB

[armcknight]

One thing I'd recommend here is to wrap whatever function we actually want to use in a function we control, to make it easier to test and potentially change later. It could be the case that strncpy is ok in some places, but not others where we'd then want strlcpy. (I wrote this paragraph before taking a deeper look at the code, and it looks like we used to have such a function, and we do have places in the code that emulate strlcpy and others that don't. So, we would need to validate the behavior after making this change).

My understanding after reading the docs is that strncpy is fine to use for copying from string buffers to characters arrays, but doesn't guarantee null termination (although does fill in the remainder of the array if the size of the source buffer is lesser than the length of the destination char array), which, I don't know why that's actually needed in a character array aside from copying it back to a buffer, which I don't think we do with any of these.

from https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/strncpy.3.html#//apple_ref/doc/man/3/strncpy:

The strncpy() function copies at most n characters from s2 into s1. If s2 is less than n characters long, the remainder of s1 is filled with `\0' characters. Otherwise, s1 is not terminated.

strlcpy is guaranteed to write at least one null terminator, and so that must be accounted for in the length parameter passed into it. I think straight replacement will result in one less character of actual data being written to accommodate that, so we might have to add a +1 to each place here?

from https://linux.die.net/man/3/strlcpy:

The strlcpy() function copies up to size - 1 characters from the NUL-terminated string src to dst, NUL-terminating the result.

I guess this is what this line of code is trying to replicate: https://github.com/getsentry/sentry-cocoa/pull/4636/files#diff-de0cc4487a50688dff672b8cd13b3901fdbf09abe93afa34b54bd561d1796fe8L1260 and I also just saw we actually already had another function to do this: https://github.com/getsentry/sentry-cocoa/pull/4636/files#diff-898d2165aedeeac3e2023d294e4bffee79b96b5c04d9d9d5be7e7593d4eb9aeeL21-L27

This is why a wrapper function is my preference, we can hide all these details and keep things consistent. If we have places that move data from char arrays back to buffers, we should also have a wrapper function for that as well.

[philprime]

Thanks @armcknight for reviewing the PR.
I haven't had a chance yet to add more written context, therefore the PR is marked as draft.

As requested in the issue and mentioned by you, we need to make sure the changes do not break the SDK. That's why I went ahead and started adding additional unit tests.

strlcpy is guaranteed to write at least one null terminator, and so that must be accounted for in the length parameter passed into it. I think straight replacement will result in one less character of actual data being written to accommodate that, so we might have to add a +1 to each place here?

I was thinking of the same, and yes we might need to extend the buffers at some places by one character, if it actually uses the full buffer length.

I also found the function strncpy_safe, but looking at the implementation it seems to work mimic the behaviour of strlcpy so I don't see a point in maintaining a duplicate implementation.

sentry-cocoa/Sources/SentryCrash/Reporting/Filters/Tools/SentryStringUtils.h

Lines 21 to 27 in 419f1d4

	static inline char *
	strncpy_safe(char *dst, const char *src, size_t n)
	{
	strncpy(dst, src, n - 1);
	dst[n - 1] = '\0';
	return dst;
	}

TL;DR: I added todos to the description what is left to do.

In total I found 7 uses of strlcpy and strncpy, so I'll quickly add my thoughts for each so far:

1. int sentry_asyncLogSetFileName(const char *filename, bool overwrite):

• Called by SentryAsyncLogWrapper with the full path of the async.log file, which is placed in the result of NSSearchPathForDirectoriesInDomains. Therefore the length of the path is controlled by the OS.
• Currently no maximum file path length is enforced → therefore the current implementation using strncpy will write up to 1024 characters leaving it unterminated, with strlcpy up to 1023 characters but definitely terminated.
• Copies the given filename to the g_logFileName, which is a 1024 char buffer.
• The g_logFileName is used by addTextLinesFromFile(..) in SentryCrashReport.c which then uses the path with open to create a file descriptor.
• I don't think the g_logFileName should ever be unterminated, so we should definitely replace it.

2. static void CPPExceptionTerminate(void):

• Uses the strncpy_safe to copy the exception description into a 1000 char long descriptionBuff.
• The description should be null terminated.
• As mentioned above, the strncpy_safe method tries to do the same as strlcpy, so I believe we should replace this.
• I will try to create a unit test with a description longer than 1000 chars, to verify the expected behaviour.

3. static void onCrash(struct SentryCrash_MonitorContext *monitorContext):

• Generates a crash report file path using sentrycrashcrs_getNextCrashReportPath, then writes it to a local variable crashReportFilePath with a size of SentryCrashFU_MAX_PATH_LENGTH = 500 characters.
• sentrycrashcrs_getNextCrashReportPath uses getCrashReportPathByID to generate the path
• getCrashReportPathByID uses snprintf which also has SentryCrashCRS_MAX_PATH_LENGTH defined as the max length.
• The generated path will be null terminated, from https://linux.die.net/man/3/snprintf:

The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.

• Therefore crashReportFilePath will always be null terminated and can safely be copied to g_lastCrashReportFilePath without changing the buffer size.
• I added tests in SentryCrashCTests.swift

4. void sentrycrashreport_writeRecrashReport(..., const *path, ...):

• This one I still need to verify/test, because it uses strncpy to copy path to a tempPath which has a length limit of SentryCrashFU_MAX_PATH_LENGTH, but then replaces the last characters with a .old extension
• I tried to write unit tests in SentryCrashCTests.swift but kind-off failed because it tries to create a monitor context from thread data, which I do not fully understand yet.

sentry-cocoa/Sources/SentryCrash/Recording/SentryCrashReport.c

Lines 1474 to 1479 in 419f1d4

	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);
	SENTRY_ASYNC_SAFE_LOG_INFO("Writing recrash report to %s", path);

5. static bool increaseDepth(FixupDepth *context, const char *name):

• Copies the given name to the context->objectPath[context->currentDepth]
• If name is NULL it sets the context->objectPath[context->currentDepth] to \0, therefore an empty but terminated string
• Based on that, I believe it is safe to assume that the context->objectPath[context->currentDepth] should always be terminated, and it is safe to use strlcpy instead of strncpy.
• The object path length is MAX_NAME_LENGTH = 100 so we might have to increase the buffer size by one, as there could be names having exactly that length.

sentry-cocoa/Sources/SentryCrash/Recording/SentryCrashReportFixer.c

Lines 54 to 68 in 419f1d4

	static bool
	increaseDepth(FixupContext *context, const char *name)
	{
	if (context->currentDepth >= MAX_DEPTH) {
	return false;
	}
	if (name == NULL) {
	*context->objectPath[context->currentDepth] = '\0';
	} else {
	strncpy(context->objectPath[context->currentDepth], name,
	sizeof(context->objectPath[context->currentDepth]));
	}
	context->currentDepth++;
	return true;
	}

6. static bool deletePathContents(const char *path, bool deleteTopLevelPathAlso) in SentryCrashFileUtils.c:

• The given path is passed to snprintf which truncates it to SentryCrashFU_MAX_PATH_LENGTH - 1 - 1 characters, then appending / and \0 and written to pathBuffer.
• The address of the end of pathBuffer is set to pathPtr to calculate the remaining buffer length and set it to pathRemainingLength.
• strncpy will use the the pathRemainingLength as the limit of characters copied to pathPtr and therefore to pathBuffer.
• The pathBuffer is then used to recursively call deletePathContents which uses snprintf again to terminate the string, therefore we can use strlcpy instead of strncpy because it will be terminated anyways.

sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashFileUtils.c

Lines 155 to 167 in 419f1d4

	int bufferLength = SentryCrashFU_MAX_PATH_LENGTH;
	char *pathBuffer = malloc((unsigned)bufferLength);
	snprintf(pathBuffer, bufferLength, "%s/", path);
	char *pathPtr = pathBuffer + strlen(pathBuffer);
	int pathRemainingLength = bufferLength - (int)(pathPtr - pathBuffer);
	for (int i = 0; i < entryCount; i++) {
	char *entry = entries[i];
	if (entry != NULL && canDeletePath(entry)) {
	strncpy(pathPtr, entry, pathRemainingLength);
	deletePathContents(pathBuffer, true);
	}
	}

• I added additional unit tests in SentryCrashFileUtils_Tests.m
• ⚠️ The current implementation might actually not work if the path is longer than SentryCrashFU_MAX_PATH_LENGTH. I will add another unit test to verify.

7. decodeElement() in SentryCrashJSONCodec.c:

• The current implementation performs manual null termination while using strncpy, so I believe it to be safe to replace it with strlcpy instead:

sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashJSONCodec.c

Lines 1259 to 1260 in 419f1d4

	strncpy(context->stringBuffer, start, len);
	context->stringBuffer[len] = '\0';

[codecov]

Codecov Report

Attention: Patch coverage is 98.62385% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.286%. Comparing base (c5fef16) to head (7c9a4ae).

Files with missing lines	Patch %	Lines
...ntryCrash/SentryCrashMonitor_CppException_Tests.mm	92.187%	5 Missing ⚠️
...Tests/SentryCrash/SentryThreadInspectorTests.swift	75.000%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4636       +/-   ##
=============================================
+ Coverage   90.631%   91.286%   +0.654%     
=============================================
  Files          621       620        -1     
  Lines        71153     71555      +402     
  Branches     25310     26028      +718     
=============================================
+ Hits         64487     65320      +833     
+ Misses        6574      6135      -439     
- Partials        92       100        +8     

Files with missing lines	Coverage Δ
Sources/Sentry/SentryAsyncSafeLog.c	100.000% <100.000%> (ø)
...rding/Monitors/SentryCrashMonitor_CPPException.cpp	75.000% <100.000%> (+49.242%)	⬆️
Sources/SentryCrash/Recording/SentryCrashC.c	79.577% <100.000%> (+11.971%)	⬆️
Sources/SentryCrash/Recording/SentryCrashReport.c	53.846% <100.000%> (+19.963%)	⬆️
...ces/SentryCrash/Recording/SentryCrashReportFixer.c	87.283% <100.000%> (ø)
...SentryCrash/Recording/Tools/SentryCrashFileUtils.c	87.764% <100.000%> (+1.882%)	⬆️
...SentryCrash/Recording/Tools/SentryCrashJSONCodec.c	87.093% <ø> (+0.697%)	⬆️
...ests/SentryTests/Recording/SentryCrashCTests.swift	100.000% <100.000%> (ø)
...ntryTests/SentryCrash/SentryCrashFileUtils_Tests.m	100.000% <100.000%> (ø)
...Tests/SentryCrash/SentryThreadInspectorTests.swift	85.947% <75.000%> (ø)
... and 1 more

... and 32 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5fef16...7c9a4ae. Read the comment docs.

[armcknight]

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?

[philprime]

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.

[armcknight]

Good work to make the changes and write the tests that cover a lot of implementation code.

We use a common pattern of sut (system under test) which you might be able to use to refactor some of the tests. I know there's a tension between DRY and self-contained test logic. Figured I'd just offer it up as food for thought.

Overall I think the changes are fine so this has my approval. I merely offer some suggestions and comments along the way.

[armcknight]

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

[philprime]

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.

[armcknight]

This happens in all the test cases, so could go into -[setUp]

[philprime]

You are right, for some reason I thought this had to be after std::set_terminate which is different per test case

[armcknight]

You might set the last character to something other than A, so that we know for a fact that we're truncating from the end of the string. Super edge casey, but it occurred to me so I figured I'd share.

[armcknight]

Copypasta

Suggested change

	// Build a 1000 + 1 character message
	// Build a 1000 character message

[armcknight]

I like this pattern. Wish there was a way to somehow do it structurally with syntax vs comments (could use labels and goto 😂 💀 more realistically, C macros, but that wouldn't work in Swift). Using XCTAssert... macros makes that part obvious I suppose. Generally, it's nice 👍🏻 I found this https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/ (the author's name is Andrew Knight 🤯)

[philprime]

High chance that article actually inspired me to use that pattern. Macros etc. could give this even more structure, but there are always cases where that could make it more complicated/harder to maintain.

[armcknight]

I prefer to not use the test- prefix for anything other than actual test methods. Maybe mock-?

Suggested change

	testHandleExceptionHandler(struct SentryCrash_MonitorContext *context)
	mockExceptionHandler(struct SentryCrash_MonitorContext *context)

[armcknight]

I'm guessing these are in service of covering SentryCrashFileUtils.c line 164 (although not all of them do), but that's why I suggested using a wrapper function that could be tested more directly. These are integration-y and have many possible failure modes.

I don't feel strongly but it wasn't clear to me exactly why they were being added until I went digging. It's a lot more test code to achieve the same ends.

[philprime]

In all honesty I don't see much of a point in having a wrapper around strlcpy as we should test it's usage, rather than it's implementation.

For this PR I tried to add test cases for all places where I changed from strncpy to strlcpy, therefore also sentrycrashfu_deleteContentsOfPath defined in SentryCrashFileUtils.h. As we want to have high test coverage anyways, I decided to add test cases for all code paths.

[armcknight]

Yep totally fair 👍🏻

[armcknight]

Suggested change

	* When the overall path length is larger than ``SentryCrashFU_MAX_PATH_LENGTH``, it the given
	* When the overall path length is larger than ``SentryCrashFU_MAX_PATH_LENGTH``, the given

[armcknight]

Good on you for encoding behavior in a test like this, but IMO this situation is something we should fix ASAP if it's happening in the wild. We should always expect to be able to deliver crash reports, and guard against situations that would prevent that and work around them whenever possible. At the very least a warning message should be available for end users to also potentially work around it, like if they provide a custom cache path that is too long. The only other thing I can see that would cause this is a bundle id that is too long (we should also only use the bundle id for mac apps; ios/tvos/visionos are all self-contained files trees per app)

[philprime]

I had a quick chat with @philipphofmann yesterday, I still need to validate if a too long file path can actually be passed down from the SentrySDK options using cacheDirectoryPath to the SentryCrashC, or if it causes issues before that.

He also mentioned a warning should be sufficient, but at this time I already created this test case to actually test the behavior with invalid paths.

In general I still believe we should also add tests to pin down expected negative behavior (if possible), so even if an edge case can hardly ever happen, we do have documentation that it can happen. If it then turns out to be an actual issue, we can change test-case, and fix the implementation until it passes again.

[armcknight]

Gratuitous use of vertical space. I'd change all such code:

Suggested change

	var appName = "SentryCrashCTests"
	.cString(using: .utf8)!
	let installDir = URL(fileURLWithPath: NSTemporaryDirectory())
	.appendingPathComponent("SentryCrashCTests-\(UUID().uuidString)")
	var installPath = installDir
	.path
	.cString(using: .utf8)!
	let expectedReportsDir = installDir
	.appendingPathComponent("Reports")
	// Smoke test the existence of the directory
	XCTAssertFalse(FileManager.default.fileExists(
	atPath: expectedReportsDir.path
	))
	var appName = "SentryCrashCTests".cString(using: .utf8)!
	let installDir = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("SentryCrashCTests-\(UUID().uuidString)")
	var installPath = installDir.path.cString(using: .utf8)!
	let expectedReportsDir = installDir.appendingPathComponent("Reports")
	// Smoke test the existence of the directory
	XCTAssertFalse(FileManager.default.fileExists(atPath: expectedReportsDir.path))