Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sector size calculation for Disk images #248

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nikitalita
Copy link

A fix for a bug mentioned in #247: implementing fixes for sector size detection for bootable images.

We either calculate the sector count based on the emulated floppy size, or we look at the boot sector of the image to see if it's a valid boot sector. If so, we retrieve the sector count from there.

When writing to the disk, if we are writing a boot image with emulation, we mark the sector count as 1 in the boot catalog.

@nikitalita nikitalita force-pushed the fix_iso_boot branch 2 times, most recently from 574400f to 462ffb6 Compare January 3, 2022 05:29
@nikitalita
Copy link
Author

??????????

@nikitalita
Copy link
Author

Bumping this again, as I still need this.

@LTRData
Copy link

LTRData commented Feb 24, 2024

I implemented your changes in my fork now:
LTRData@ca3b9c3

I plan to publish new NuGet packages with this fix soon. Thanks a lot for your contribution!

Could you take a look at this test case? It fails after this modification. I am not sure whether the test was actually wrong previously, or if anything is wrong now.
https://github.com/LTRData/DiscUtils/blob/ca3b9c38e615d3eb3a24ead202ee230a39af28c7/Tests/LibraryTests/Iso9660/BuilderTest.cs#L52

@nikitalita
Copy link
Author

Not really sure, but we were writing the boot sectors wrong in the first place, it’s likely that the test had similarly wrong parameters.

@LTRData
Copy link

LTRData commented Feb 24, 2024

Okay, sounds like it makes sense. I'll try to figure out how this could be tested in some other way.

@LTRData
Copy link

LTRData commented Feb 25, 2024

Okay, fixed the test case and built new nuget packages. Packages from my fork are prefixed with LTRData., so for example LTRData.DiscUtils.Iso9660. Note also that there are some breaking changes because that fork has evolved a bit in last few years since it was forked from this repo.

@EricZimmerman
Copy link

.net 8 targets as well for nuget? =)

@LTRData
Copy link

LTRData commented Feb 25, 2024

.net 8 targets as well for nuget? =)

Yes!

@EricZimmerman
Copy link

excellent. KAPE is getting ported to your library for v2

@nikitalita

This comment was marked as outdated.

@LTRData
Copy link

LTRData commented Feb 26, 2024

I went back and actually checked; The test was correctly failing and I'm not calculating the boot sectors correctly for hard disk emulation. @LTRData I would advise reverting the test change and committing the suggestion above.

Thanks! But I guess we also need to fix FixExtents method as well, where it sets _bootEntry.SectorCount = 1 for all other BootMediaType values than NoEmulation? Should it set it to actual number of sectors for HardDisk type boot images as well, or should it be calculated from some kind of boot record like for floppy images?

@nikitalita
Copy link
Author

Uggggh, I think see the issue. I had thought that I was calculating the sector count wrong because the test was reporting 512, but the test assumed the boot IMAGE was first because it used to write IMAGE, then CATALOG (this does not conform to the El Torito spec, it's supposed to be CATALOG then IMAGE). So the test read the boot catalog size, which is always 1 sector, 512 bytes.

So, to sum up, I think you have the tests fixed correctly and you don’t have to change anything. I will investigate with real world examples to make sure.

@LTRData
Copy link

LTRData commented Mar 2, 2024

Having looked more into this, I think that there is some logic missing i SetBootImage for harddisk emulation. We would need something corresponding to what mkisofs does here:
https://github.com/Distrotech/cdrtools/blob/8adb0d06e070464f13d017b1de7187bf23dacd87/mkisofs/eltorito.c#L374

Also, OpenBootImage would need some similar logic to find actual image size. We cannot use size of the boot entry for hard disk emulation either, because it is always 1. But we also cannot calculate the size in a way similar to how it is done for floppies because there really is no geometry data like that in the MBR for hard disks. I think I'll look more into this before doing more changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants