v2.0.3 accesses NULL pointer when no more free space

[Mohawi]

We mocked a blockdevice in a way that it falsifies data to program when littleFS tries to commit a crc after writing a file. While littleFS looks for new free space (which it can't find) it produces a hard fault by a nullptr access.

lfs_dir_fetch calls lfs_dir_fetchmatch with a NULL callback, stating that this is ok since an invalid bit is set. It turns out, that after some iterations of calling lfs_dir_fetch the tag loaded within lfs_dir_fetchmatch becomes 0, gets matched and the NULL callbackpointer is invoked.

I'm proposing following fix:

static int lfs_dir_fetch(lfs_t *lfs,
        lfs_mdir_t *dir, const lfs_block_t pair[2]) {
    // note, mask=-1, tag=-1 can never match a tag since this
    // pattern has the invalid bit set
    return lfs_dir_fetchmatch(lfs, dir, pair, -1, -1, NULL, NULL, NULL);
}

instead of

static int lfs_dir_fetch(lfs_t *lfs,
        lfs_mdir_t *dir, const lfs_block_t pair[2]) {
    // note, mask=-1, tag=0 can never match a tag since this
    // pattern has the invalid bit set
    return lfs_dir_fetchmatch(lfs, dir, pair, -1, 0, NULL, NULL, NULL);
}

[mon]

This just bit us on a real filesystem with plenty of space left. Not entirely sure what caused the tag type 0 to be returned (corruption of some sort?), but NPE is obviously bad behaviour.

Bump for the PR to be looked at again?

[geky]

I haven't been able to reproduce the issue yet, but I think we should go ahead and merge the fix since it could be causing problems in many hard-to-hit situations.

I went ahead and split out your PR @Mohawi and will merge it when I can. Thanks for raising this. https://github.com/ARMmbed/littlefs/pull/337

[mon]

Would it be worth wrapping the call in a if(cb != NULL) as well? The possibility of a tag being -1 should be nil because it's invalid, but I'm wondering if any edge cases would make it show up.

[geky]

We could put a LFS_ASSERT(cb != NULL) there, though I don't think that would be much different than a null pointer exception if the condition is hit.

[mon]

In some scenarios our flash has died and thrown an assert, I made LFS_ASSERT return -1 to safely recover instead of aborting the entire thing. An assert would be much nicer!

[geky]

This condition isn't based on the state of the flash at all. Just before the fetch routine exits if it hits an invalid tag: https://github.com/ARMmbed/littlefs/pull/337/files#diff-630e6e312bb168dc6903ffc8496ddb9cR819-R824

So the only thing the assert would be checking against is that the code is written correctly.

If you wanted to get fancy, you could instrument your hard-fault handler to also perform recovery : )

[mon]

Ah, I missed the invalid check prior - with this PR and that check, an assert would be useless as you say!

[geky]

As long as no one writes any more bugs!