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.

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

memory: ensure to check allocation results #3084

Closed

Conversation

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Oct 2, 2018

The result of a memory allocation should always be checked, as we may run under memory pressure where even a small allocation can fail. This adds checking and error handling to a few cases where the allocation wasn't checked for success. Also bumps the copyright years on affected files.

This is based on manual static analysis/code reading, no live bugs have been observed.

The result of a memory allocation should always be checked, as we may
run under memory pressure where even a small allocation can fail. This
adds checking and error handling to a few cases where the allocation
wasn't checked for success. Also bumps the copyright years on affected
files.
@jay
Copy link
Member

@jay jay commented Oct 3, 2018

you don't need this pattern if(foo)free(foo) just free(foo). also the second one i think the lock needs to be freed as well and for that you do need if(foo)NXMutexFree(foo)

@bagder
bagder approved these changes Oct 3, 2018
Spotted by Jay Satiro
@danielgustafsson
Copy link
Member Author

@danielgustafsson danielgustafsson commented Oct 3, 2018

you don't need this pattern if(foo)free(foo) just free(foo).

Right, I was mainly following the style of the surrounding code there, to make the flow easier to read.

also the second one i think the lock needs to be freed as well and for that you do need if(foo)NXMutexFree(foo)

Nice catch, have pushed a fixup commit with this.

In the unlikely event that strdup() fails for (""), ensure to error
our with the appropriate errorcode. Also move freeing path up to
above the strdup() call since there is little point in keeping it
around across the strdup, and the separation makes for more readable
code.
@danielgustafsson
Copy link
Member Author

@danielgustafsson danielgustafsson commented Oct 3, 2018

Pulled in the related strdup() fix from #3078, will squash these three commits into one before pushing.

@jay
Copy link
Member

@jay jay commented Oct 3, 2018

Right, I was mainly following the style of the surrounding code there, to make the flow easier to read.

There is no need for a conditional there, just free(foo) is fine. Other than that LGTM

@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.