Fixing malloc without free causing memory leak

[caroaguilar]

Fix memory leak happening every time a user exception is reported. We found this while using the bugsnag-react-native SDK and profiling the app with Xcode Instruments. From there we were able to track this bug down to bugsnag-cocoa SDK.

By adding the missing free() to the SDK locally we see the leak fixed and not showing in Instruments.

The malloc in question is found here https://github.com/bugsnag/bugsnag-cocoa/blob/master/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m#L317

Since bsg_kscrash_reportUserException from above file could or could not return, I thought it was better to free the resource in BSG_KSCrashSentry_User.c which is final in call hierarchy.

Changeset

Adding the respectively free() for the malloc

Review

• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

[kattrali]

Hi @caroaguilar! Thank you for the patch. Looking at the code in question, it looks like a leak. This function (bsg_kscrashsentry_reportUserException) is only called from one place - +[BSG_KSCrash reportUserException:...] which is performing the allocation.

bugsnag-cocoa/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m

Lines 303 to 317 in c8e5a5a

	- (void)reportUserException:(NSString *)name
	reason:(NSString *)reason
	originalException:(NSException *)exception
	handledState:(NSDictionary *)handledState
	appState:(NSDictionary *)appState
	callbackOverrides:(NSDictionary *)overrides
	metadata:(NSDictionary *)metadata
	config:(NSDictionary *)config
	discardDepth:(int)depth
	terminateProgram:(BOOL)terminateProgram {
	const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding];
	const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding];
	NSArray *addresses = [exception callStackReturnAddresses];
	NSUInteger numFrames = [addresses count];
	uintptr_t *callstack = malloc(numFrames * sizeof(*callstack));

As the allocation is not happening in bsg_kscrashsentry_reportUserException, and since use of the array is completed when bsg_kscrashsentry_reportUserException finishes running, it would be easier to follow if the free() was instead in +[BSG_KSCrash reportUserException:...] just after bsg_kscrash_reportUserException is called (around line 334).

bugsnag-cocoa/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m

Lines 324 to 334 in c8e5a5a

	bsg_kscrash_reportUserException(cName, cReason,
	callstack, numFrames,
	[handledState[@"currentSeverity"] UTF8String],
	[self encodeAsJSONString:handledState],
	[self encodeAsJSONString:overrides],
	[self encodeAsJSONString:metadata],
	[self encodeAsJSONString:appState],
	[self encodeAsJSONString:config],
	depth,
	terminateProgram);

It would also mean that the free() only needs to happen once, instead of checking for early returns.

The test suite is failing, but it looks like just a missing semicolon after free().

[caroaguilar]

Hi @kattrali, thank you for your quick response and review!

Makes total sense, already applied the requested changes.

Thanks :)