embed.rs: Added error check when ISO file incomplete

[RishabhSaini]

Follows on Issue#869 and Jira cos-1556
Giving clearer error message when ISO file is not complete
Implements jlebon's suggestion
last file's offset + file length <= ISO length
The new error message displayed when incomplete ISO file detected is: Error: ISO Image is wrong and can't be modified

[bgilbert]

Thanks for pursuing this!

I don't think the karg embed area parser is the right place to put this check, because any other code that reads data from the ISO could encounter the same problem. Instead, let's do this validation when initially parsing the ISO.

Jonathan's suggestion requires reading all of the ISO image's directories, and per the above we'd do it every time we parsed an ISO. Instead, I'd suggest comparing the Volume Space Size field in the Primary Volume Descriptor to the file size, which should save a bunch of I/O. That wouldn't detect the case where an invalid ISO image claims to have files beyond the Volume Space Size, but we're only trying to catch a truncated download, not arbitrary data corruption, so that should be okay. Note that the actual file size will likely be larger than the Volume Space Size, because isohybrid adds an EFI partition table after the end of the filesystem.

CI is failing because you need to run rustfmt.

[RishabhSaini]

Currently this is what the Primary Volume Descriptor returns
PrimaryVolumeDescriptor { system_id: "LINUX", volume_id: "rhcos-412.86.202206301758-0", root: Directory { name: ".", address: Address(29), length: 2048 } }
To access the field VolumeSpaceSize, I have tried to read the specific bytes by using the address(29) as offset and adding 80 which is the offset of the VolumeSpaceSize as mentioned in the ISO9660 wiki. Then using read_exact to read the length 8 bytes from the offset.
Is this the correct method to get the VolumeSpaceSize field from PrimaryVolumeDescriptor?

[bgilbert]

The address is measured in sectors, not bytes. (Filesystem data structures often do this, since they point to things at sector boundaries. The lower bits of the address would be wasted otherwise.)

coreos-installer has an existing parse function that creates the PrimaryVolumeDescriptor struct. The in-memory struct doesn't include every field in the on-disk structure, since we don't use most of them. However, since we're now going to care about an additional field, we should add it to the in-memory struct and to the parse function.

[bgilbert]

Overall structure looks good. 🎉 Some cleanups.

[bgilbert]

Also, let's update test_primary_volume_descriptor, and maybe add a test based on open_iso() that verifies that we correctly detect a truncated image.

[bgilbert]

Oh, also, we've switched to updating docs/release-notes.md from individual PRs, so let's add a note there.

[jlebon]

Maybe just something like

Suggested change

	- Added an error check for detecting an incomplete ISO image
	- iso: Detect incomplete ISO files

?

[bgilbert]

Maybe "iso: Improve error for incomplete ISO files"? We were detecting it before, just with an obscure message.

[RishabhSaini]

Updated

[cgwalters]

I think in the case of a corrupted ISO this multiplication could overflow? Probably worth something like:

Suggested change

	if primary.volume_space_size * ISO9660_SECTOR_SIZE as u64 > length {
	if primary.volume_space_size.checked_mul(ISO9660_SECTOR_SIZE as u64).ok_or_else(|| anyhow!("Invalid ISO volume size"))? > length

or so?

[RishabhSaini]

we're only trying to catch a truncated download, not arbitrary data corruption

Yes you are correct but I assumed the above to be true

[bgilbert]

volume_space_size is 32 bits on disk, so this can't overflow.

[bgilbert]

Looks great!