diskfs / go-diskfs

MIT License
494 stars 112 forks source link

Finalize error with a combination of RockRidge and ElTorito #215

Closed pierrec closed 3 months ago

pierrec commented 3 months ago

The following code fails with: unable to calculate size of directory for .: could not calculate child boot.catalog entry size .: could not convert to dirEntry: error getting extensions for IEEE_P1282 at path boot.catalog: error reading file /var/folders/lb/nhsppbxs5_v27pn_x3kf66wc0000gn/T/diskfs_iso1583485680/boot.catalog: lstat /var/folders/lb/nhsppbxs5_v27pn_x3kf66wc0000gn/T/diskfs_iso1583485680/boot.catalog: no such file or directory

fs, _ := iso9660.Create(f, 0, 0, blocksize, "")
file, _ := fs.OpenFile("/limine-bios-cd.bin", os.O_CREATE|os.O_RDWR)
defer file.Close()
_ = fs.Finalize(iso9660.FinalizeOptions{
    RockRidge: true,
    ElTorito: &iso9660.ElTorito{
        Entries: []*iso9660.ElToritoEntry{
            {
                Platform:  iso9660.BIOS,
                Emulation: iso9660.NoEmulation,
                BootFile:  "/limine-bs-cd.bin",
                BootTable: true,
                LoadSize:  4,
            },
        },
    },
})
deitch commented 3 months ago

This is consumed here. boot.catalog is used based on the following logic:

        bootcat = options.ElTorito.generateCatalog()
        // figure out where to save it on disk
        catname := options.ElTorito.BootCatalog
        switch {
        case catname == "" && options.RockRidge:
            catname = elToritoDefaultCatalogRR
        case catname == "":
            catname = elToritoDefaultCatalog
        }

Basically:

Since you did set RockRidge, and did not provide the option, it sets that as the catalog name.

It then prepends that to the list of root files:

        catSize := int64(len(bootcat))
        catEntry = &finalizeFileInfo{
            content:   bootcat,
            size:      catSize,
            path:      catname,
            name:      path.Base(catname),
            shortname: shortname,
            extension: extension,
            blocks:    calculateBlocks(catSize, fs.blocksize),
        }
        // make it the first file
        files = append([]*finalizeFileInfo{catEntry}, files...)

Since that finalizeFileInfo has a non-empty content property, it should write it out from memory here:

        if e.content == nil {
   .....
        } else {
            copied = len(e.content)
            if _, err = f.WriteAt(e.content, writeAt); err != nil {
                return fmt.Errorf("failed to write content of %s to disk: %v", e.path, err)
            }
        }

and should not be reading from the temporary space, which the error you showed above implies it is doing.

Can you turn the above into a simple ready-to-run main program that reproduces it? In here or in a gist is fine.

pierrec commented 3 months ago

I have a test that triggers the issue :)


func TestFinalizeElToritoWithRockRidge(t *testing.T) {
    blocksize := int64(2048)
    t.Run("valid", func(t *testing.T) {
        f, err := os.CreateTemp("", "iso_finalize_test")
        defer os.Remove(f.Name())
        if err != nil {
            t.Fatalf("Failed to create tmpfile: %v", err)
        }
        fs, err := iso9660.Create(f, 0, 0, blocksize, "")
        if err != nil {
            t.Fatalf("Failed to iso9660.Create: %v", err)
        }

        file, err := fs.OpenFile("/limine-bios-cd.bin", os.O_CREATE|os.O_RDWR)
        if err != nil {
            t.Fatalf("Failed to iso9660.Filesystem.OpenFile: %v", err)
        }
        defer file.Close()

        err = fs.Finalize(iso9660.FinalizeOptions{
            RockRidge: true,
            ElTorito: &iso9660.ElTorito{
                Entries: []*iso9660.ElToritoEntry{
                    {
                        Platform:  iso9660.BIOS,
                        Emulation: iso9660.NoEmulation,
                        BootFile:  "/limine-bios-cd.bin",
                        BootTable: true,
                        LoadSize:  4,
                    },
                },
            },
        })
        if err != nil {
            t.Fatal("unexpected error fs.Finalize({RockRidge: true})", err)
        }
    })
}
pierrec commented 3 months ago

The output is:

--- FAIL: TestFinalizeElToritoWithRockRidge (0.00s)
    --- FAIL: TestFinalizeElToritoWithRockRidge/valid (0.00s)
        finalize_test.go:525: unexpected error fs.Finalize({RockRidge: true}) unable to calculate size of directory for .: could not calculate child boot.catalog entry size .: could not convert to dirEntry: error getting extensions for IEEE_P1282 at path boot.catalog: error reading file /var/folders/lb/nhsppbxs5_v27pn_x3kf66wc0000gn/T/diskfs_iso236828151/boot.catalog: lstat /var/folders/lb/nhsppbxs5_v27pn_x3kf66wc0000gn/T/diskfs_iso236828151/boot.catalog: no such file or directory
deitch commented 3 months ago

Problem is here:

        for _, e := range fs.suspExtensions {
            ext, err := e.GetFileExtensions(path.Join(fs.workspace, fi.path), isSelf, isParent)
            if err != nil {
                return nil, fmt.Errorf("error getting extensions for %s at path %s: %v", e.ID(), fi.path, err)
            }
            ext2, err := e.GetFinalizeExtensions(fi)
            if err != nil {
                return nil, fmt.Errorf("error getting finalize extensions for %s at path %s: %v", e.ID(), fi.path, err)
            }
            ext = append(ext, ext2...)
            de.extensions = append(de.extensions, ext...)
        }

It tries to get the file extensions from the file on disk, not recognizing that this is an in-memory entry.

deitch commented 3 months ago

Try out branch in #216 please @pierrec

deitch commented 3 months ago

Maybe hold on that, needs some restructuring yet.

deitch commented 3 months ago

OK, ready. Go ahead and try it.

pierrec commented 3 months ago

OK, will try soon.