Sets all allocated buffer pointers to NULL after they are free()-ed. …

[evpopov]

…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

[BenBE]

It looks like you missed some occurances in bd/lfs_rambd.c and bd/lfs_testbd.c. Can you take a look?

[evpopov]

Thanks for pointing that out @BenBE. I had never even looked into that directory.

[BenBE]

NP. I also only found those while looking for lfs_free while checking your PR for completeness. ;-)

[geky]

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.

[BenBE]

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.

Why do you think this?

All this does is expecting the used standard library to be(have) standard compliant.

lfs_unmount() -> lfs_unmount() is undefined behavior. You can't really expect littlefs to do anything sane in this situation.

Sorry to disagree here: The sane thing is: Unmount with the first call (if previously mounted), report failure for the second call.

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.

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 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.

At a minimum you could tag functions which require a non-NULL pointer (like lfs_mount and lfs_unmount) with __attribute__((nonnull)) so modern compilers can do the NULL checks at compile-time (or warn you if you potentially pass NULL pointers to something that needs to be non-NULL).

C is a foot gun, but there are well-known safety precautions you can take to minimize the risk for you and others.
Nonetheless, most people ignore them anyway …

[geky]

Sorry to disagree here: The sane thing is: Unmount with the first call (if previously mounted), report failure for the second call.

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).

Provided you have a somewhat sane set of pre-conditions

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.

[BenBE]

To the point of maybe being opinionated here …

Sorry to disagree here: The sane thing is: Unmount with the first call (if previously mounted), report failure for the second call.

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).

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 lfs_unmount twice in a row is not handled in a somewhat sane way, this should be addressed (in a different PR). Always try to think of possible abuses of your public interface and ensure the code reacts in a sane way. Calling APIs in a situation when they should not be called, should result in an appropriate error, not memory corruption.

Provided you have a somewhat sane set of pre-conditions

You could argue the pre-condition is that each lfs_unmount should have a matching lfs_mount.

No: lfs_unmount should receive a properly mounted file system object/reference.

That such a reference (under current API) can legally only be obtained by lfs_mount is a result of the API. If you imagine some future version of LFS introducing a new (hypothetical) "lfs_create_blank" function that hands back a reference to the newly mounted filesystem (it just created anew). This would instantly violate your "must call lfs_mount before lfs_unmount rule. Thus: Describe the state an object is expected to be in, not the list of things to do before you reach there …

Nit-picking:

lfs_mount(&fs);
memset(&fs, 0, sizeof(fs));
lfs_unmount(&fs);

technically satisfies the "mount before unmount" rule … ;-)

But you are right null internal buffers is a situation that should never happen, and it would be easy to assert for.

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.

The issue I have is when debugging asserts increase the code size of the "release" build.

In my experience, hinting the compiler about NULL pointers and checks often reduces code size.
Asserts should only handle implicit assumptions of the code about its data structures (e.g. checking for an argument to be in the right range). Memory validity should IMO not be handled in asserts.

--

Enough ranting: Please demonstrate a bug in LFS that is worsened by avoiding UB (from invalid pointers) …

[evpopov]

I completely agree with @BenBE about public-facing sides of APIs and how they should be sane ( I'd add - bulletproof )
If this library is interfaced with a command-line interpreter, someone could always call the API in a weird way like that and when they do, corrupting memory is not acceptable in my mind.
The only other way out in this situation is to have another mount/unmount function defined so that user code calls this extra layer, the extra layer ensures lfs_mount/lfs_unmount are paired and only then calls them. This sounds like extra complexity that is way too easy to avoid by including this very clean and simple PR.

@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.