Skip to content

Conversation

@TheLoneWolfling
Copy link

lfs_traverse has no protection against infinite loops. Consider what happens if a directory entry has dir.d.tail == cwd... you end up hanging forever. Relevant section of code:

int lfs_traverse(lfs_t *lfs, int (*cb)(void*, lfs_block_t), void *data) {
    ...
    lfs_dir_t dir;
    lfs_block_t cwd[2]
    ...
    while (true) {
        ...
        lfs_dir_fetch(lfs, &dir, cwd);
        ...
        cwd[0] = dir.d.tail[0];
        cwd[1] = dir.d.tail[1];
        ...
    }
}

This is mostly a theoretical issue - this can only really happen if the on-flash data is already in an invalid state. However, as littlefs is supposed to be failsafe...

Two fixes come to mind:

  1. Keep a counter. If you ever traverse >= the total number of blocks, you know that you've hit a loop. (This may take a while... but this isn't supposed to happen unless something has gone wrong anyway, and it's still better than just hanging forever.)
  2. Use something like the tortose-and-hare algorithm. (Advance the hare, do the callback, exit if done, panic if hare == tortose, advance the hare, do the callback, exit if done, panic if hare == tortose, advance the tortose, panic if hare == tortose. Repeat.) Probably overkill.

This change implements #1.

lfs_traverse has no protection against infinite loops. Consider what happens if a directory entry has dir.d.tail == cwd... you end up hanging forever. [Relevant section of code:](https://github.com/ARMmbed/littlefs/blob/195075819e05a9ce8568d3d98363f2a6f19ed436/lfs.c#L2294)

```
int lfs_traverse(lfs_t *lfs, int (*cb)(void*, lfs_block_t), void *data) {
    ...
    lfs_dir_t dir;
    lfs_block_t cwd[2]
    ...
    while (true) {
        ...
        lfs_dir_fetch(lfs, &dir, cwd);
        ...
        cwd[0] = dir.d.tail[0];
        cwd[1] = dir.d.tail[1];
        ...
    }
}
```

This is mostly a theoretical issue - this can only really happen if the on-flash data is already in an invalid state. However, as littlefs is supposed to be failsafe...

Two fixes come to mind:

1. Keep a counter. If you ever traverse >= the total number of blocks, you know that you've hit a loop. (This may take a while... but this isn't supposed to happen unless something has gone wrong _anyway_, and it's still better than just hanging forever.)
2. Use something like the tortose-and-hare algorithm. (Advance the hare, do the callback, exit if done, panic if hare == tortose, advance the hare, do the callback, exit if done, panic if hare == tortose, advance the tortose, panic if hare == tortose. Repeat.) Probably overkill.

This change implements littlefs-project#1.
@TheLoneWolfling
Copy link
Author

TheLoneWolfling commented Jan 27, 2019

Note: there is an analogous issue in lfs_parent - although it's harder to hit, it can be hit from lfs_deorphan.

Ditto, there's an analogous issue with the open file loop in lfs_traverse.

@geky
Copy link
Member

geky commented Jan 28, 2019

Hi @TheLoneWolfling, thanks for creating a PR. I'm going to wait on this until v2 just because a lot of the code is going to change. But you're right it's probably a good idea to have these checks on every loop.

Related #98

@TheLoneWolfling
Copy link
Author

Ok. Is it alright if I start looking at v2? Or should I hold off?

@geky
Copy link
Member

geky commented Jan 31, 2019

Sure thing! It may be rough around the edges, but any feedback is welcome :)

@geky
Copy link
Member

geky commented Feb 24, 2020

Ok! Only a year and one major version late. I've added proper loop detection/recovery to #372.

I'll go ahead and close this once #372 is merged. But let me know if you see anything that needs fixing.

Thanks again for the contribution!

@geky geky closed this Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants