Repository: diskfs/go-diskfs
Pull request #195 (2023-12-12 20:08:37)

squashfs: fix directory listings
This fixes directory listings sometimes giving the error

    could not parse directory header: header was 3 bytes, less than minimum 12

This appears to be because empty directories can indeed have no
entries despite what the spec says.

This introduces a test checking that we can recurse the test squashfs
file correctly - this fails without this patch.


ncw: Note that the test introduced here won't pass until https://github.com/diskfs/go-diskfs/pull/193 is merged. Maybe I should have put both commits in the same PR.
ncw: Note that I'm working on a larger patch to fix up the reading of files out of squashfs which is quite broken in places. Its looking a lot better but it isn't there quite yet!
deitch: Do you need to rebase this on #193 now that it is merged in?
deitch: > Note that I'm working on a larger patch to fix up the reading of files out of squashfs which is quite broken in places. It's looking a lot better but it isn't there quite yet!

Thank you, I am looking forward to it. Also to how the tests change and see what they missed (or just weren't implemented 😆 )
ncw: > Do you need to rebase this on #193 now that it is merged in?

I've rebased this and I've also fixed the test I broke!

> > Note that I'm working on a larger patch to fix up the reading of files out of squashfs which is quite broken in places. It's looking a lot better but it isn't there quite yet!
> 
> Thank you, I am looking forward to it. Also to how the tests change and see what they missed (or just weren't implemented 😆 )

I think the tests are very well implemented :-) I've been working on more end-to-end tests so unpacking actual archives created by mksquashfs and there are a lot of corner cases!

I'm hoping to get this all passing shortly but if I get stuck I may push it anyway  so you can see the progress and maybe give hints!

deitch: I am confused. Don't we need to parse the header? You are saying it is possible to have an empty directory without any header at all (even though the spec says otherwise). Then ho would we know if the directory is valid?
ncw: > I am confused. Don't we need to parse the header?

I think that the lines of code I deleted 57-60 are effectively repeated at 63-66 when `pos == 0` so this bit of code is just a check to see whether there is a header or not.

https://github.com/diskfs/go-diskfs/blob/e5b494094885d06b3a1ef8fd9334ed62480aa376/filesystem/squashfs/directory.go#L56-L66

> You are saying it is possible to have an empty directory without any header at all (even though the spec says otherwise). Then ho would we know if the directory is valid?

If you look in the `testdata/file.sqs` you'll find such an empty directory at `/a/b/c/d/` . If you undo my fix by putting those lines back in then the new test fails with:

> squashfs_test.go:340: error reading directory /a/b/c/d: could not read directory at path /a/b/c/d: could not get entries: could not get entries: could not get entries: could not get entries: unable to read directory from table: could not parse directory header: header was 3 bytes, less than minimum 12

I don't know for sure whether that fix is correct, there could be a problem somewhere else, but that is what I came up with to fix the error.

[The spec](https://dr-emann.github.io/squashfs/#directory-table) I've been looking at says

> A header must have at least one entry. A header must not be followed by more than 256 entries. If there are more entries, a new header is emitted.

Which I took to mean that the directory shouldn't be empty, but looking at it at face value it says **if** a header exists **then** it must have at least one entry. Where the case under discussion here is if the header **does not** exist.

A check for "A header must have at least one entry" would look something like this I think

```diff
diff --git a/filesystem/squashfs/directory.go b/filesystem/squashfs/directory.go
index b559af5..1e8628f 100644
--- a/filesystem/squashfs/directory.go
+++ b/filesystem/squashfs/directory.go
@@ -60,6 +60,9 @@ func parseDirectory(b []byte) (*directory, error) {
 		if err != nil {
 			return nil, fmt.Errorf("could not parse directory header: %v", err)
 		}
+		if directoryHeader.count == 0 {
+			return nil, fmt.Errorf("corrupted directory, must have at least one entry")
+		}
 		if directoryHeader.count > maxDirEntries {
 			return nil, fmt.Errorf("corrupted directory, had %d entries instead of max %d", directoryHeader.count, maxDirEntries)
 		}
```

However that is almost pointless since `directoryHeader.count` is a `uint32` that gets 1 added to it here but I guess it checks something useful so I could add it to the patch if you want.

https://github.com/diskfs/go-diskfs/blob/e5b494094885d06b3a1ef8fd9334ed62480aa376/filesystem/squashfs/directory.go#L147

I think the directoryHeader being missing is the only way to encode an empty directory otherwise (which squashfs clearly can as the `/a/b/c/d/` directory unpacks as an empty directory) - what do you think?
deitch: > I think that the lines of code I deleted 57-60 are effectively repeated at 63-66 when pos == 0 so this bit of code is just a check to see whether there is a header or not.

Rereading the code, I see that makes sense. Our only code after the (deleted) check is looping through the dir entries, `for pos := 0; pos+dirHeaderSize < len(b);` but if `len(b)` is less than 12, it will have no loops, and then just return. So this works.

So, yeah, I think you are right. Let's say, "this is the fault of unclear spec writers, we did our best, and now @ncw fixed it for us." 😄 

Is this ready to go in?
ncw: > So, yeah, I think you are right. Let's say, "this is the fault of unclear spec writers, we did our best, and now @ncw fixed it for us." 😄

:-)

> Is this ready to go in?

I've updated the patch to add the extra check in and add a test for that check. While doing that I noticed that the for loop range needed fixing too! I updated the commit message with a summary of what we learnt above (I hadn't thought it through properly until writing the above so I thought it deserved to go in the commit message!

It is ready to go now if you are happy :-)



Patch:
diff --git a/filesystem/squashfs/directory.go b/filesystem/squashfs/directory.go
index 804d339..ce7627b 100644
--- a/filesystem/squashfs/directory.go
+++ b/filesystem/squashfs/directory.go
@@ -54,16 +54,15 @@ type directoryEntryGroup struct {
 
 // parse raw bytes of a directory to get the contents
 func parseDirectory(b []byte) (*directory, error) {
-	// must have at least one header
-	if _, err := parseDirectoryHeader(b); err != nil {
-		return nil, fmt.Errorf("could not parse directory header: %v", err)
-	}
 	entries := make([]*directoryEntryRaw, 0)
-	for pos := 0; pos+dirHeaderSize < len(b); {
+	for pos := 0; pos+dirHeaderSize <= len(b); {
 		directoryHeader, err := parseDirectoryHeader(b[pos:])
 		if err != nil {
 			return nil, fmt.Errorf("could not parse directory header: %v", err)
 		}
+		if directoryHeader.count == 0 {
+			return nil, fmt.Errorf("corrupted directory, must have at least one entry")
+		}
 		if directoryHeader.count > maxDirEntries {
 			return nil, fmt.Errorf("corrupted directory, had %d entries instead of max %d", directoryHeader.count, maxDirEntries)
 		}


Test patch:
diff --git a/filesystem/squashfs/const_internal_test.go b/filesystem/squashfs/const_internal_test.go
index e69f814..d1aa323 100644
--- a/filesystem/squashfs/const_internal_test.go
+++ b/filesystem/squashfs/const_internal_test.go
@@ -16,6 +16,7 @@ import (
 const (
 	Squashfsfile             = "./testdata/file.sqs"
 	SquashfsUncompressedfile = "./testdata/file_uncompressed.sqs"
+	SquashfsfileListing      = "./testdata/list.txt"
 )
 
 // first header
diff --git a/filesystem/squashfs/directory_internal_test.go b/filesystem/squashfs/directory_internal_test.go
index 666c22d..4390aed 100644
--- a/filesystem/squashfs/directory_internal_test.go
+++ b/filesystem/squashfs/directory_internal_test.go
@@ -26,6 +26,9 @@ var (
 	testDirectory = &directory{
 		entries: testDirectoryEntries,
 	}
+	testDirectoryTableWithNoEntries = []byte{
+		0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
+	}
 )
 
 func TestParseDirectoryHeader(t *testing.T) {
@@ -115,7 +118,8 @@ func TestParseDirectory(t *testing.T) {
 		err error
 	}{
 		{testDirectoryTable, testDirectory, nil},
-		{testDirectoryTable[:10], nil, fmt.Errorf("could not parse directory header: header was 10 bytes, less than minimum 12")},
+		{testDirectoryTable[:10], &directory{}, nil},
+		{testDirectoryTableWithNoEntries, nil, fmt.Errorf("corrupted directory, must have at least one entry")},
 	}
 	for i, tt := range tests {
 		dir, err := parseDirectory(tt.b)
diff --git a/filesystem/squashfs/squashfs_test.go b/filesystem/squashfs/squashfs_test.go
index f27f15c..90c5ef0 100644
--- a/filesystem/squashfs/squashfs_test.go
+++ b/filesystem/squashfs/squashfs_test.go
@@ -1,6 +1,7 @@
 package squashfs_test
 
 import (
+	"bufio"
 	"fmt"
 	"io"
 	"os"
@@ -294,6 +295,71 @@ func TestSquashfsRead(t *testing.T) {
 	}
 }
 
+// Check the directory listing is correct
+func TestSquashfsCheckListing(t *testing.T) {
+	// read the directory listing in
+	var listing = map[string]struct{}{}
+	flist, err := os.Open(squashfs.SquashfsfileListing)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer flist.Close()
+	scanner := bufio.NewScanner(flist)
+	for scanner.Scan() {
+		line := scanner.Text()
+		line = strings.TrimPrefix(line, ".")
+		if line == "/" {
+			continue
+		}
+		listing[line] = struct{}{}
+	}
+	if err := scanner.Err(); err != nil {
+		t.Fatal(err)
+	}
+
+	// Open the squash file
+	f, err := os.Open(squashfs.Squashfsfile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+	fi, err := f.Stat()
+	if err != nil {
+		t.Fatal(err)
+	}
+	// create the filesystem
+	fs, err := squashfs.Read(f, fi.Size(), 0, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var list func(dir string)
+	list = func(dir string) {
+		fis, err := fs.ReadDir(dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, fi := range fis {
+			p := path.Join(dir, fi.Name())
+			if _, found := listing[p]; found {
+				delete(listing, p)
+			} else {
+				t.Errorf("Found unexpected path %q in listing", p)
+			}
+			if fi.IsDir() {
+				list(p)
+			}
+		}
+	}
+
+	list("/")
+
+	// listing should be empty now
+	for p := range listing {
+		t.Errorf("Didn't find %q in listing", p)
+	}
+}
+
 func TestSquashfsCreate(t *testing.T) {
 	tests := []struct {
 		blocksize int64
diff --git a/filesystem/squashfs/testdata/buildtestsqs.sh b/filesystem/squashfs/testdata/buildtestsqs.sh
index 39fc0cf..6b8674d 100755
--- a/filesystem/squashfs/testdata/buildtestsqs.sh
+++ b/filesystem/squashfs/testdata/buildtestsqs.sh
@@ -25,4 +25,7 @@ mksquashfs . /data/file.sqs
 
 # uncompressed version
 mksquashfs . /data/file_uncompressed.sqs -noI -noD -noF
+
+# create listing to check
+find . > /data/list.txt
 EOF
diff --git a/filesystem/squashfs/testdata/list.txt b/filesystem/squashfs/testdata/list.txt
new file mode 100644
index 0000000..2e48817
--- /dev/null
+++ b/filesystem/squashfs/testdata/list.txt
@@ -0,0 +1,516 @@
+/
+/README.md
+/a
+/a/b
+/a/b/c
+/a/b/c/d
+/attrfile
+/emptylink
+/foo
+/foo/filename_0
+/foo/filename_1
+/foo/filename_10
+/foo/filename_100
+/foo/filename_101
+/foo/filename_102
+/foo/filename_103
+/foo/filename_104
+/foo/filename_105
+/foo/filename_106
+/foo/filename_107
+/foo/filename_108
+/foo/filename_109
+/foo/filename_11
+/foo/filename_110
+/foo/filename_111
+/foo/filename_112
+/foo/filename_113
+/foo/filename_114
+/foo/filename_115
+/foo/filename_116
+/foo/filename_117
+/foo/filename_118
+/foo/filename_119
+/foo/filename_12
+/foo/filename_120
+/foo/filename_121
+/foo/filename_122
+/foo/filename_123
+/foo/filename_124
+/foo/filename_125
+/foo/filename_126
+/foo/filename_127
+/foo/filename_128
+/foo/filename_129
+/foo/filename_13
+/foo/filename_130
+/foo/filename_131
+/foo/filename_132
+/foo/filename_133
+/foo/filename_134
+/foo/filename_135
+/foo/filename_136
+/foo/filename_137
+/foo/filename_138
+/foo/filename_139
+/foo/filename_14
+/foo/filename_140
+/foo/filename_141
+/foo/filename_142
+/foo/filename_143
+/foo/filename_144
+/foo/filename_145
+/foo/filename_146
+/foo/filename_147
+/foo/filename_148
+/foo/filename_149
+/foo/filename_15
+/foo/filename_150
+/foo/filename_151
+/foo/filename_152
+/foo/filename_153
+/foo/filename_154
+/foo/filename_155
+/foo/filename_156
+/foo/filename_157
+/foo/filename_158
+/foo/filename_159
+/foo/filename_16
+/foo/filename_160
+/foo/filename_161
+/foo/filename_162
+/foo/filename_163
+/foo/filename_164
+/foo/filename_165
+/foo/filename_166
+/foo/filename_167
+/foo/filename_168
+/foo/filename_169
+/foo/filename_17
+/foo/filename_170
+/foo/filename_171
+/foo/filename_172
+/foo/filename_173
+/foo/filename_174
+/foo/filename_175
+/foo/filename_176
+/foo/filename_177
+/foo/filename_178
+/foo/filename_179
+/foo/filename_18
+/foo/filename_180
+/foo/filename_181
+/foo/filename_182
+/foo/filename_183
+/foo/filename_184
+/foo/filename_185
+/foo/filename_186
+/foo/filename_187
+/foo/filename_188
+/foo/filename_189
+/foo/filename_19
+/foo/filename_190
+/foo/filename_191
+/foo/filename_192
+/foo/filename_193
+/foo/filename_194
+/foo/filename_195
+/foo/filename_196
+/foo/filename_197
+/foo/filename_198
+/foo/filename_199
+/foo/filename_2
+/foo/filename_20
+/foo/filename_200
+/foo/filename_201
+/foo/filename_202
+/foo/filename_203
+/foo/filename_204
+/foo/filename_205
+/foo/filename_206
+/foo/filename_207
+/foo/filename_208
+/foo/filename_209
+/foo/filename_21
+/foo/filename_210
+/foo/filename_211
+/foo/filename_212
+/foo/filename_213
+/foo/filename_214
+/foo/filename_215
+/foo/filename_216
+/foo/filename_217
+/foo/filename_218
+/foo/filename_219
+/foo/filename_22
+/foo/filename_220
+/foo/filename_221
+/foo/filename_222
+/foo/filename_223
+/foo/filename_224
+/foo/filename_225
+/foo/filename_226
+/foo/filename_227
+/foo/filename_228
+/foo/filename_229
+/foo/filename_23
+/foo/filename_230
+/foo/filename_231
+/foo/filename_232
+/foo/filename_233
+/foo/filename_234
+/foo/filename_235
+/foo/filename_236
+/foo/filename_237
+/foo/filename_238
+/foo/filename_239
+/foo/filename_24
+/foo/filename_240
+/foo/filename_241
+/foo/filename_242
+/foo/filename_243
+/foo/filename_244
+/foo/filename_245
+/foo/filename_246
+/foo/filename_247
+/foo/filename_248
+/foo/filename_249
+/foo/filename_25
+/foo/filename_250
+/foo/filename_251
+/foo/filename_252
+/foo/filename_253
+/foo/filename_254
+/foo/filename_255
+/foo/filename_256
+/foo/filename_257
+/foo/filename_258
+/foo/filename_259
+/foo/filename_26
+/foo/filename_260
+/foo/filename_261
+/foo/filename_262
+/foo/filename_263
+/foo/filename_264
+/foo/filename_265
+/foo/filename_266
+/foo/filename_267
+/foo/filename_268
+/foo/filename_269
+/foo/filename_27
+/foo/filename_270
+/foo/filename_271
+/foo/filename_272
+/foo/filename_273
+/foo/filename_274
+/foo/filename_275
+/foo/filename_276
+/foo/filename_277
+/foo/filename_278
+/foo/filename_279
+/foo/filename_28
+/foo/filename_280
+/foo/filename_281
+/foo/filename_282
+/foo/filename_283
+/foo/filename_284
+/foo/filename_285
+/foo/filename_286
+/foo/filename_287
+/foo/filename_288
+/foo/filename_289
+/foo/filename_29
+/foo/filename_290
+/foo/filename_291
+/foo/filename_292
+/foo/filename_293
+/foo/filename_294
+/foo/filename_295
+/foo/filename_296
+/foo/filename_297
+/foo/filename_298
+/foo/filename_299
+/foo/filename_3
+/foo/filename_30
+/foo/filename_300
+/foo/filename_301
+/foo/filename_302
+/foo/filename_303
+/foo/filename_304
+/foo/filename_305
+/foo/filename_306
+/foo/filename_307
+/foo/filename_308
+/foo/filename_309
+/foo/filename_31
+/foo/filename_310
+/foo/filename_311
+/foo/filename_312
+/foo/filename_313
+/foo/filename_314
+/foo/filename_315
+/foo/filename_316
+/foo/filename_317
+/foo/filename_318
+/foo/filename_319
+/foo/filename_32
+/foo/filename_320
+/foo/filename_321
+/foo/filename_322
+/foo/filename_323
+/foo/filename_324
+/foo/filename_325
+/foo/filename_326
+/foo/filename_327
+/foo/filename_328
+/foo/filename_329
+/foo/filename_33
+/foo/filename_330
+/foo/filename_331
+/foo/filename_332
+/foo/filename_333
+/foo/filename_334
+/foo/filename_335
+/foo/filename_336
+/foo/filename_337
+/foo/filename_338
+/foo/filename_339
+/foo/filename_34
+/foo/filename_340
+/foo/filename_341
+/foo/filename_342
+/foo/filename_343
+/foo/filename_344
+/foo/filename_345
+/foo/filename_346
+/foo/filename_347
+/foo/filename_348
+/foo/filename_349
+/foo/filename_35
+/foo/filename_350
+/foo/filename_351
+/foo/filename_352
+/foo/filename_353
+/foo/filename_354
+/foo/filename_355
+/foo/filename_356
+/foo/filename_357
+/foo/filename_358
+/foo/filename_359
+/foo/filename_36
+/foo/filename_360
+/foo/filename_361
+/foo/filename_362
+/foo/filename_363
+/foo/filename_364
+/foo/filename_365
+/foo/filename_366
+/foo/filename_367
+/foo/filename_368
+/foo/filename_369
+/foo/filename_37
+/foo/filename_370
+/foo/filename_371
+/foo/filename_372
+/foo/filename_373
+/foo/filename_374
+/foo/filename_375
+/foo/filename_376
+/foo/filename_377
+/foo/filename_378
+/foo/filename_379
+/foo/filename_38
+/foo/filename_380
+/foo/filename_381
+/foo/filename_382
+/foo/filename_383
+/foo/filename_384
+/foo/filename_385
+/foo/filename_386
+/foo/filename_387
+/foo/filename_388
+/foo/filename_389
+/foo/filename_39
+/foo/filename_390
+/foo/filename_391
+/foo/filename_392
+/foo/filename_393
+/foo/filename_394
+/foo/filename_395
+/foo/filename_396
+/foo/filename_397
+/foo/filename_398
+/foo/filename_399
+/foo/filename_4
+/foo/filename_40
+/foo/filename_400
+/foo/filename_401
+/foo/filename_402
+/foo/filename_403
+/foo/filename_404
+/foo/filename_405
+/foo/filename_406
+/foo/filename_407
+/foo/filename_408
+/foo/filename_409
+/foo/filename_41
+/foo/filename_410
+/foo/filename_411
+/foo/filename_412
+/foo/filename_413
+/foo/filename_414
+/foo/filename_415
+/foo/filename_416
+/foo/filename_417
+/foo/filename_418
+/foo/filename_419
+/foo/filename_42
+/foo/filename_420
+/foo/filename_421
+/foo/filename_422
+/foo/filename_423
+/foo/filename_424
+/foo/filename_425
+/foo/filename_426
+/foo/filename_427
+/foo/filename_428
+/foo/filename_429
+/foo/filename_43
+/foo/filename_430
+/foo/filename_431
+/foo/filename_432
+/foo/filename_433
+/foo/filename_434
+/foo/filename_435
+/foo/filename_436
+/foo/filename_437
+/foo/filename_438
+/foo/filename_439
+/foo/filename_44
+/foo/filename_440
+/foo/filename_441
+/foo/filename_442
+/foo/filename_443
+/foo/filename_444
+/foo/filename_445
+/foo/filename_446
+/foo/filename_447
+/foo/filename_448
+/foo/filename_449
+/foo/filename_45
+/foo/filename_450
+/foo/filename_451
+/foo/filename_452
+/foo/filename_453
+/foo/filename_454
+/foo/filename_455
+/foo/filename_456
+/foo/filename_457
+/foo/filename_458
+/foo/filename_459
+/foo/filename_46
+/foo/filename_460
+/foo/filename_461
+/foo/filename_462
+/foo/filename_463
+/foo/filename_464
+/foo/filename_465
+/foo/filename_466
+/foo/filename_467
+/foo/filename_468
+/foo/filename_469
+/foo/filename_47
+/foo/filename_470
+/foo/filename_471
+/foo/filename_472
+/foo/filename_473
+/foo/filename_474
+/foo/filename_475
+/foo/filename_476
+/foo/filename_477
+/foo/filename_478
+/foo/filename_479
+/foo/filename_48
+/foo/filename_480
+/foo/filename_481
+/foo/filename_482
+/foo/filename_483
+/foo/filename_484
+/foo/filename_485
+/foo/filename_486
+/foo/filename_487
+/foo/filename_488
+/foo/filename_489
+/foo/filename_49
+/foo/filename_490
+/foo/filename_491
+/foo/filename_492
+/foo/filename_493
+/foo/filename_494
+/foo/filename_495
+/foo/filename_496
+/foo/filename_497
+/foo/filename_498
+/foo/filename_499
+/foo/filename_5
+/foo/filename_50
+/foo/filename_500
+/foo/filename_51
+/foo/filename_52
+/foo/filename_53
+/foo/filename_54
+/foo/filename_55
+/foo/filename_56
+/foo/filename_57
+/foo/filename_58
+/foo/filename_59
+/foo/filename_6
+/foo/filename_60
+/foo/filename_61
+/foo/filename_62
+/foo/filename_63
+/foo/filename_64
+/foo/filename_65
+/foo/filename_66
+/foo/filename_67
+/foo/filename_68
+/foo/filename_69
+/foo/filename_7
+/foo/filename_70
+/foo/filename_71
+/foo/filename_72
+/foo/filename_73
+/foo/filename_74
+/foo/filename_75
+/foo/filename_76
+/foo/filename_77
+/foo/filename_78
+/foo/filename_79
+/foo/filename_8
+/foo/filename_80
+/foo/filename_81
+/foo/filename_82
+/foo/filename_83
+/foo/filename_84
+/foo/filename_85
+/foo/filename_86
+/foo/filename_87
+/foo/filename_88
+/foo/filename_89
+/foo/filename_9
+/foo/filename_90
+/foo/filename_91
+/foo/filename_92
+/foo/filename_93
+/foo/filename_94
+/foo/filename_95
+/foo/filename_96
+/foo/filename_97
+/foo/filename_98
+/foo/filename_99
+/goodlink
+/hardlink
+/random
+/random/largefile
+/zero
+/zero/largefile