On 14 June 2014 09:09, Tobias Stoeckmann <[email protected]> wrote:
> Hi,
>
> fsck_msdos is prone to an infinite loop if cluster chains in the
> filesystem are infinite: FAT handles clusters as linked lists, and at
> worst this list can be cyclic.
>
> Android included a fix from Samsung for this issue, with the commit
> b6ee08aadb580341a4d80943741b80de16a88b5d:
>
> https://android.googlesource.com/platform/external/fsck_msdos/+/b6ee08aadb580341a4d80943741b80de16a88b5d
>
> I disagree on it though. Instead of finding the offending cluster,
> their code randomly takes one of them.
>
> In revision 1.9, mickey@ fixed this issue almost 14 years ago. His fix
> was incomplete though, still leading to an infinite loop later on.
> But his fix made it possible to retrieve the length of the cyclic list
> up to the offending cluster. Therefore, my diff will stop the for-loop
> if it hits that length, spotting the offending cluster. The user can
> choose if the cluster chain should be removed or truncated
> _at the offending cluster_.
>
>
> Tobias
>
> Index: fat.c
> ===================================================================
> RCS file: /cvs/src/sbin/fsck_msdos/fat.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 fat.c
> --- fat.c 14 Jun 2014 12:33:07 -0000 1.21
> +++ fat.c 14 Jun 2014 12:50:18 -0000
> @@ -364,10 +364,10 @@ checkfat(struct bootblock *boot, struct
> continue;
>
> /* follow the chain to its end (hopefully) */
> - for (p = head;
> + for (len = fat[head].length, p = head;
> (n = fat[p].next) >= CLUST_FIRST && n <
> boot->NumClusters;
> p = n)
An assignment inside the condition makes me cry, but I guess that's
not related to *this* problem.
> - if (fat[n].head != head)
> + if (fat[n].head != head || len-- < 2)
The '2' is a bit magical, perhaps worth a comment somewhere.
> break;
> if (n >= CLUST_EOFS)
> continue;
> @@ -381,6 +381,12 @@ checkfat(struct bootblock *boot, struct
> if (n < CLUST_FIRST || n >= boot->NumClusters) {
> pwarn("Cluster chain starting at %u ends with cluster
> out of range (%u)\n",
> head, n);
> + ret |= tryclear(boot, fat, head, &fat[p].next);
> + continue;
> + }
> + if (head == fat[n].head) {
> + pwarn("Cluster chain starting at %u loops at cluster
> %u\n",
> + head, p);
> ret |= tryclear(boot, fat, head, &fat[p].next);
> continue;
> }
>
Otherwise it seems sane to me. ok krw@
.... Ken
