fsck.fat: "Bad short file name ()." for root directory on FAT32

[dosfstools]

fsck.fat: "Bad short file name ()." for root directory on FAT32

[dmo2118]

git bisect says it first showed up in 4d01db7.

[pali]

@fstirlitz this is your commit.

In 4d01db7 is broken -S option, see:

$ ./src/fsck.fat -S disk.fat 
./src/fsck.fat: invalid option -- 'S'
Usage: ./src/fsck.fat [OPTIONS] DEVICE
Check FAT filesystem on DEVICE for errors.

Options:
  -a              automatically repair the filesystem
  -A              toggle Atari variant of the FAT filesystem
  -b              make read-only boot sector check
  -c N            use DOS codepage N to decode short file names (default: 850)
  -d PATH         drop file with name PATH (can be given multiple times)
  -f              salvage unused chains to files
  -l              list path names
  -n              no-op, check non-interactively without changing
  -p              same as -a, for compat with other *fsck
  -r              interactively repair the filesystem (default)
  -S              disallow spaces in the middle of short file names
  -t              test for bad clusters
  -u PATH         try to undelete (non-directory) file that was named PATH (can be
                    given multiple times)
  -v              verbose mode
  -V              perform a verification pass
  --variant=TYPE  handle variant TYPE of the filesystem
  -w              write changes to disk immediately
  -y              same as -a, for compat with other *fsck
  --help          print this message

Also in that commit is added no_spaces_in_sfns into fatlabel.c even it does not modify fatlabel. This is another mistake.

[fstirlitz]

Here's the fix. I'd squash it on top of my last commit.

[pali]

https://github.com/fstirlitz/dosfstools/blob/3fc495c68b59fdfbbf8e1ac53307cb4393ab86cb/src/check.c#L158-L160

This is probably not the best way how to check for root entry. What if other invalid FAT entry would have offset set to zero?

[fstirlitz]

The offset field of a DOS_FILE struct is set by add_file according to one of its arguments, which in turn is computed internally as the offset of the direntry relative to the start of the entire file system image (or set to 0, for the synthetic FAT32 root node). That value is never read from the file system image. The same check is used in auto_rename, rename_file and check_file; I think it's fine.

By the way, I think it'd be wise to ask why the test suite failed to catch this mistake.

[pali]

Ok, then it seems fine. And why test suite failed is a good question. There must be missing such test which creates an empty filesystem and then runs on it fsck.

[fstirlitz]

Actually I do know the answer to that one:

./tests$ XXD_FOUND=yes srcdir=. sh -x ./test-mkfs ./referenceFAT32.mkfs 
+ [ 1 -ne 1 ]
+ basename ./referenceFAT32.mkfs .mkfs
+ testname=referenceFAT32
+ [ yes != yes ]
+ . ./referenceFAT32.mkfs
+ ARGS=-n TESTFAT32
+ SIZE=1024000
+ CMP_LIMIT=10M
+ echo Test referenceFAT32
Test referenceFAT32
+ rm -f referenceFAT32.out referenceFAT32.refimg
+ xxd -r ./referenceFAT32.xxd referenceFAT32.refimg
+ run_mkfs -C -v --invariant -n TESTFAT32 referenceFAT32.out 1024000
+ ../src/mkfs.fat -C -v --invariant -n TESTFAT32 referenceFAT32.out 1024000
mkfs.fat 4.1+git (2017-01-24)
Auto-selecting FAT32 for large filesystem
referenceFAT32.out has 255 heads and 63 sectors per track,
hidden sectors 0x0000;
logical sector size is 512,
using 0xf8 media descriptor, with 2048000 sectors;
drive number 0x80;
filesystem has 2 32-bit FATs and 8 sectors per cluster.
FAT size is 2000 sectors, and provides 255496 clusters.
There are 32 reserved sectors.
Volume ID is 1234abcd, volume label TESTFAT32  .
+ echo

+ echo Comparing...
Comparing...
+ limitarg=
+ [ -n 10M ]
+ limitarg=--bytes=10M
+ cmp --bytes=10M referenceFAT32.out referenceFAT32.refimg
+ success=0
+ [ 0 -eq 0 ]
+ echo

+ echo Testing fsck...
Testing fsck...
+ run_fsck -n referenceFAT32.out
+ ../src/fsck.fat -n referenceFAT32.out
fsck.fat 4.1+git (2017-01-24)
/
  Bad short file name ().
  Auto-renaming it.
  Renamed to 
referenceFAT32.out: 1 files, 1/255496 clusters
+ success=0
+ rm -f referenceFAT32.out referenceFAT32.refimg
+ [ 0 -eq 2 ]
+ exit 0

Apparently fsck.fat with the -n option attempts to 'fix' the issue without reflecting the fact that it detected an error in the program's exit code.

[dmo2118]

e9a7460 makes "Bad short file name" go away for me.

[pali]

@fstirlitz Are you going to prepare a pull request with this fix?

[fstirlitz]

I guess @andreasbombe can already cherry-pick it without involving GitHub pull requests:

$ git fetch [email protected]:fstirlitz/dosfstools.git
$ git cherry-pick --no-commit e9a7460
$ git commit --amend --no-edit
$ git push --force

(Yes, git push --force is frowned upon, but I think it's justifiable in this case, while the faulty commit is still the tip of the branch and relatively fresh.)

[pali]

@andreasbombe Can you fix it?

[andreasbombe]

I will take that fix once I figured out the reason for the test suite failure to fail. It's not supposed to return zero when it sees an error, fixed or not.

Edit: Ok that could be because it isn't actually "fixing" it by changing anything and if nothing is changed, fsck returns 0 (i.e. it is not recording whether an error was detected, only whether something was or would be changed).

[pali]

It relates to issue #89 (Use standard fsck exit codes in fsck.fat).

[andreasbombe]

Not quite, the error codes that are there should have made the test fail the same that all other checks in the test suite work.

The thing is that the error is code is set on whether there were changes at the end rather than on when errors were detected. And the error was detected and "fixed" yet no changes were made.

[andreasbombe]

I pushed the fix. I couldn't get at the commit to cherry-pick so I created the commits myself.

Also, the reason the error was not showing up in the test suite is because the when the rename functions were called to "fix" the issue, they would detect they are called on the root directory and return without doing anything. Since the filesystem wasn't changed, fsck returned success.