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

Do not call lfs_free if the buffer to free is NULL #776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cdesjardins
Copy link
Contributor

This can happen if lfs_malloc fails, or if the file doesn't exist, all "goto cleanup" clauses before and including the the lfs_malloc will cause this.

This can happen if lfs_malloc fails, or if the file doesn't exist,
all "goto cleanup" clauses before and including the the lfs_malloc
will cause this.
@vonj
Copy link

vonj commented Feb 8, 2023

This can happen if lfs_malloc fails, or if the file doesn't exist, all "goto cleanup" clauses before and including the the lfs_malloc will cause this.

Can this be triggered in a unit test, or would a malloc fail somehow have to be provoked?

@BenBE
Copy link

BenBE commented Feb 8, 2023

Please note my comment in this discussion: free with NULL is a no-op. Only thing that littlefs should guarantee is to properly initialize all its memory so unset pointers are NULL instead of random values..

@evpopov
Copy link

evpopov commented Mar 29, 2023

I suggest this pull request be closed in lieu of #790 that I just created.
@cdesjardins, nothing personal, I've just been procrastinating after my discussion with @BenBE and just now managed to create a PR that addresses the same issue. I also covered the lfs_deinit() function in my PR which I believe covers all uses the lfs_free().

@geky
Copy link
Member

geky commented Apr 18, 2023

It's worth noting @cdesjardins's case is a bit different from #790, since the case of the "goto cleanup" in lfs_file_open can be reached in normal operation.

That being said, as @BenBE points out, free with NULL is usually a no-op, and is guaranteed to be so in a standard-conforming stdlib.

And, if free does not support NULL, the lfs_free function is under user control if they create their own lfs_util.h file. I believe this would be the correct place to add such a NULL check.

@geky geky added the needs minor version new functionality only allowed in minor versions label Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs minor version new functionality only allowed in minor versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants