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

Jump to bottom

Fix unchecked malloc #5431

Merged
merged 4 commits into from
Apr 22, 2020
Merged

Fix unchecked malloc #5431

merged 4 commits into from
Apr 22, 2020

Conversation

morganchen12
Copy link
Contributor

Fixes #5428.
@morganchen12 morganchen12 requested a review from samedson April 21, 2020 19:38
@morganchen12 morganchen12 mentioned this pull request Apr 21, 2020
Closed
samedson
samedson reviewed Apr 21, 2020
View reviewed changes
Crashlytics/Crashlytics/Models/Record/FIRCLSReportAdapter.m Outdated
error:&error]);
if (error) {
FIRCLSErrorLog(@"Failed to read from %@ with error: %@", clsRecords[i], error);
if (files != NULL) {
Copy link
Contributor

@samedson samedson Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

More of a style thing, but I like to keep the happy path code in the least-indent, and use if statements to check for issues and immediately return.

That way later code inside the method that may depend on files being non-null doesn't have a chance to run.

Can we change this to:

if (files == NULL) {
return apple_payload;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
samedson
samedson reviewed Apr 21, 2020
View reviewed changes
Crashlytics/Crashlytics/Models/Record/FIRCLSReportAdapter.m
if (pbBytes != NULL) {
memcpy(pbBytes->bytes, [data bytes], data.length);
pbBytes->size = (pb_size_t)data.length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - can we just return NULL when pbBytes == null?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
samedson
samedson requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@samedson samedson left a comment

Choose a reason for hiding this comment

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

Just a stylistic thing
@morganchen12 morganchen12 requested a review from samedson April 21, 2020 22:09
samedson
samedson approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@samedson samedson left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 🛳
@morganchen12 morganchen12 merged commit 62a8002 into master Apr 22, 2020
@morganchen12 morganchen12 deleted the morganchen/crash branch April 22, 2020 16:16
ryanwilson pushed a commit that referenced this pull request Apr 24, 2020
@morganchen12 @ryanwilson
Fix unchecked malloc (#5431)
9a86a49 
* Fix unchecked malloc

* update changelogs

* style

* Code review comments
@firebase firebase locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: crashlytics cla: yes
4 participants
@morganchen12 @samedson @googlebot @google-oss-bot