-
Notifications
You must be signed in to change notification settings - Fork 807
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
Sets all allocated buffer pointers to NULL after they are free()-ed. … #790
base: master
Are you sure you want to change the base?
Conversation
…This prevents calling free() with random memory addresses. Issue is easily reproduced by: lfs_mount() -> lfs_unmount() -> lfs_unmount()
It looks like you missed some occurances in |
Thanks for pointing that out @BenBE. I had never even looked into that directory. |
NP. I also only found those while looking for |
Hi @evpopov, first thanks for creating a PR. I don't think this should go in as is, and I think the goal is a bit out of what can be expected from a C library. lfs_unmount() -> lfs_unmount() is undefined behavior. You can't really expect littlefs to do anything sane in this situation. Consider calling lfs_unmount() without lfs_mount() beforehand. For better or worse, C is not a language where you can always protect against these sort of mistakes. At most this situation should assert iff asserts are enabled, but I'm not sure how that could be implemented without code cost in the general case. |
Why do you think this? All this does is expecting the used standard library to be(have) standard compliant.
Sorry to disagree here: The sane thing is: Unmount with the first call (if previously mounted), report failure for the second call.
Provided you have a somewhat sane set of pre-conditions (all pointers are valid or NULL), this would most likely just detect something is wrong and report back failure to unmount. If you allow any pointer (valid and invalid) you are prone to shoot your feet.
At a minimum you could tag functions which require a non-NULL pointer (like C is a foot gun, but there are well-known safety precautions you can take to minimize the risk for you and others. |
To be clear, that's not what this PR does. I think that would be better behavior, but this PR silently avoids memory corruption that could otherwise be detected by Valgrind or other tooling (though I appreciate the PR).
You could argue the pre-condition is that each lfs_unmount should have a matching lfs_mount. But you are right null internal buffers is a situation that should never happen, and it would be easy to assert for. The issue I have is when debugging asserts increase the code size of the "release" build. |
To the point of maybe being opinionated here …
If you need Valgrind to verify the correctness of your code, there are far bigger issues that should be addressed. Also: if skipping an assert affects the correctnes of your code, then this shouldn't be an assert in the first place. Thus: If calling
No: That such a reference (under current API) can legally only be obtained by Nit-picking: lfs_mount(&fs);
memset(&fs, 0, sizeof(fs));
lfs_unmount(&fs); technically satisfies the "mount before unmount" rule … ;-)
Ref to above: If dropping that check influences correctnes (Surprise: it does), this should be no assert, but actual source OR (as mentioned) a hint to the compiler.
In my experience, hinting the compiler about NULL pointers and checks often reduces code size. -- Enough ranting: Please demonstrate a bug in LFS that is worsened by avoiding UB (from invalid pointers) … |
I completely agree with @BenBE about public-facing sides of APIs and how they should be sane ( I'd add - bulletproof ) @geky you are absolutely correct by saying that this PR silently avoids memory corruption. I simply think that it's worth it to make the caller's job easier by not having them keep or check state..... and now that I say this, I have to admit that I do have those additional functions. If I don't, calling my unmount() multiple times messes up my heap and calling my mount() function multiple times will keep reallocating memory without freeing it. It would be cool if I didn't have to do this. |
…This prevents calling free() with random memory addresses.
Issue is easily reproduced by: lfs_mount() -> lfs_unmount() -> lfs_unmount()
This has been discussed in #764 and #776