Skip to content

Fix a few issues detected with static code analysis - #131

Closed
tsautereau-anssi wants to merge 3 commits into
GrapheneOS:masterfrom
tsautereau-anssi:tsautereau/fixes
Closed

Fix a few issues detected with static code analysis#131
tsautereau-anssi wants to merge 3 commits into
GrapheneOS:masterfrom
tsautereau-anssi:tsautereau/fixes

Conversation

@tsautereau-anssi

@tsautereau-anssi tsautereau-anssi commented Feb 10, 2021

Copy link
Copy Markdown
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
Comment thread h_malloc.c
}
}

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

@thestinger thestinger Feb 10, 2021

Copy link
Copy Markdown
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

thestinger commented Feb 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

thestinger commented Feb 10, 2021

Copy link
Copy Markdown
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

tsautereau-anssi commented Feb 11, 2021

Copy link
Copy Markdown
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