Infinite loop in get_sector_list with damaged .doc file #11

New Issue

2020-08-05T15:39:50Z

rossj commented

2020-08-05 15:39:50 +00:00

(Migrated from github.com)

Hi there. I've come across a problematic .doc file that is causing an infinite loop in get_sector_list.

It looks like the 2nd half of this .doc file is all null, so it is definitely damaged & invalid, but it would be nice to avoid the infinite loop.

In this specific case, the loop starts off with j = 0, which results in the next j value being read from sectors[312], which is all null bytes due to the file corruption. This results in an infinite loop with j = 0.

I noticed that the chkd array is not being checked. Adding if (chkd[j]) break; at the top of the loop avoids the infinite loop and results in a later exception. Perhaps it's better to throw immediately inside the loop?

Hi there. I've come across a problematic .doc file that is causing an infinite loop in `get_sector_list`. It looks like the 2nd half of this .doc file is all null, so it is definitely damaged & invalid, but it would be nice to avoid the infinite loop. In this specific case, the loop starts off with `j = 0`, which results in the next `j` value being read from `sectors[312]`, which is all null bytes due to the file corruption. This results in an infinite loop with j = 0. I noticed that the `chkd` array is not being checked. Adding `if (chkd[j]) break;` [at the top of the loop](https://github.com/SheetJS/js-cfb/blob/7c1980741a3a07aa218721cee0f5355ffb143303/bits/45_readfat.js#L24) avoids the infinite loop and results in a later exception. Perhaps it's better to throw immediately inside the loop?

SheetJSDev commented

2020-08-08 19:27:57 +00:00

(Migrated from github.com)

For the test suite, can you share an example file?

That code block was spun out of the make_sector_list function, which used the chkd variable to note if we've already built a chain that used the block in question and seen to note if we've already seen a given block when we build up a specific chain. If you'd like to submit a PR, remove the chkd references in make_sector_list and copy over the seen lines.

PS: In general, half the file being null isn't necessarily a problem (if those are treated as empty FAT sectors).

For the test suite, can you share an example file? That code block was spun out of the `make_sector_list` function, which used the `chkd` variable to note if we've already built a chain that used the block in question and `seen` to note if we've already seen a given block when we build up a specific chain. If you'd like to submit a PR, remove the `chkd` references in `make_sector_list` and copy over the `seen` lines. PS: In general, half the file being null isn't necessarily a problem (if those are treated as empty FAT sectors).

Sign in to join this conversation.