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 a few issues detected with static code analysis #131

Closed
wants to merge 3 commits into from
Closed

Fix a few issues detected with static code analysis #131

wants to merge 3 commits into from

Conversation

tsautereau-anssi
Copy link
Contributor

Static code analysis of hardened_malloc flagged some small issues. This pull request proposes fixes for three of them, and I'll open an issue to discuss the remaining ones, if that's OK for you.

@tsautereau-anssi
Check return value of memory_map_fixed()
4a5b3a3 
All other calls to memory_map_fixed() are checked, and this one may also
theoretically fail.
@tsautereau-anssi
Fix wrong pointer being checked for NULL
e333655 
It's the region pointer that can be NULL here, and p was checked at the
beginning of the function. Also fix the test accordingly.
@tsautereau-anssi
Fix wrong pointer being checked for NULL in ternary
3c392a7 
It's the region pointer that can be NULL here, and p was checked at the
beginning of the function.
thestinger
thestinger reviewed Feb 10, 2021
View reviewed changes
h_malloc.c
@@ -903,7 +903,10 @@ static int regions_grow(void) {
}
}

memory_map_fixed(ra->regions, ra->total * sizeof(struct region_metadata));
Copy link
Member

Choose a reason for hiding this comment

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

This call shouldn't be checked because failure doesn't impact whether the function succeeded. It doesn't really matter if it succeeds. The point of doing this is to free memory.

@thestinger
Copy link
Member

thestinger commented Feb 10, 2021 •
edited
Loading

I've cherry-picked the other 2 commits. I reworded the commit messages to explain what was wrong in both cases.

The malloc_usable_size fix makes it abort as intended rather than getting a guaranteed access fault.

The malloc_object_size fix is more important since it's needed for that function to work as intended as it's not supposed to abort when passing in an invalid pointer. The object size functions still need more work and testing before we can start using them. GrapheneOS used to have an implementation that was being actively used for the previous allocator but we haven't yet reimplemented the features using this.

@thestinger
Copy link
Member

The memory_map_fixed call you're changing is for freeing memory when possible. If it fails, that means the memory wasn't freed. That shouldn't lead to a memory allocation failure. There's nothing that can or should be done about that kind of thing failing.

@thestinger thestinger closed this Feb 10, 2021
@tsautereau-anssi
Copy link
Contributor Author

The memory_map_fixed call you're changing is for freeing memory when possible. If it fails, that means the memory wasn't freed. That shouldn't lead to a memory allocation failure. There's nothing that can or should be done about that kind of thing failing.

Alright, maybe a comment would help clarify this for future source code reviews/analyses, but thanks for the explanation!

@tsautereau-anssi tsautereau-anssi mentioned this pull request Feb 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thestinger thestinger thestinger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsautereau-anssi @thestinger