composefs/status: resolve rollback entry correctly

[chaserhkj]

Previous implementation had undefined behavior and was coincidentally correct under conditions where no rollback was performed, see #1887

Matches deployment entries in composefs deploy folder that are neither staged nor booted against entires defined in /boot to find out rollback entry.

Fixes #1887

[gemini-code-assist]

Code Review

This pull request provides a solid fix for correctly identifying the rollback deployment in composefs status. The previous implementation, which assumed any non-booted/non-staged deployment was the rollback entry, was flawed and could lead to incorrect behavior, especially in the presence of stale deployment data. The new approach of cross-referencing deployments with the actual bootloader configuration (both BLS and Grub entries) is much more robust and correctly handles the logic. The changes are well-contained and the logic is sound. I have one suggestion to refactor a loop to be more idiomatic and improve clarity.

[chaserhkj]

@cgwalters @Johan-Liebert1 Can anyone take a look at this change? I feel this change makes sense even without reproduction of #1887 since current implemented behavior relies on undefined behaviors from the underlying library/syscall ( syscall opendir from Dir::read_dir ) and that could be counted as a bug on itself.

[Johan-Liebert1]

since current implemented behavior relies on undefined behaviors from the underlying library/syscall

Could you please highlight the UB that might occur?

I might be getting this wrong since I'm unable to reproduce #1887, but wouldn't bootc status now fail at
https://github.com/bootc-dev/bootc/pull/1888/changes#diff-0c2404bde8d45d9665ce40004291992c9503391924082c74ef6ef515e1eedf50R728?

[chaserhkj]

UB comes from these code that reads the entries from directory /sysroot/state/deploy:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 545 to 549 in ad60763

	let deployments = storage
	.physical_root
	.read_dir(STATE_DIR_RELATIVE)
	.with_context(|| format!("Reading sysroot {STATE_DIR_RELATIVE}"))?;

The read_dir function doc describes the order of the returned entry list to be UB.
Later in the same function, this list with undefined order is enumerated through:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 573 to 621 in ad60763

	for depl in deployments {
	let depl = depl?;
	let depl_file_name = depl.file_name();
	let depl_file_name = depl_file_name.to_string_lossy();
	// read the origin file
	let config = depl
	.open_dir()
	.with_context(|| format!("Failed to open {depl_file_name}"))?
	.read_to_string(format!("{depl_file_name}.origin"))
	.with_context(|| format!("Reading file {depl_file_name}.origin"))?;
	let ini = tini::Ini::from_string(&config)
	.with_context(|| format!("Failed to parse file {depl_file_name}.origin as ini"))?;
	let boot_entry =
	boot_entry_from_composefs_deployment(storage, ini, depl_file_name.to_string()).await?;
	// SAFETY: boot_entry.composefs will always be present
	let boot_type_from_origin = boot_entry.composefs.as_ref().unwrap().boot_type;
	match boot_type {
	Some(current_type) => {
	if current_type != boot_type_from_origin {
	anyhow::bail!("Conflicting boot types")
	}
	}
	None => {
	boot_type = Some(boot_type_from_origin);
	}
	};
	if depl.file_name() == booted_composefs_digest.as_ref() {
	host.spec.image = boot_entry.image.as_ref().map(|x| x.image.clone());
	host.status.booted = Some(boot_entry);
	continue;
	}
	if let Some(staged_deployment_id) = &staged_deployment_id {
	if depl_file_name == staged_deployment_id.trim() {
	host.status.staged = Some(boot_entry);
	continue;
	}
	}
	host.status.rollback = Some(boot_entry);
	}

Within this loop, host.status.booted and host.status.staged are both set and early continues the loop, and the last statement catches all fall-through cases and sets host.status.rollback.

But if there are more than two entires in /sysroot/state/deploy that are neither booted nor staged, the loop still goes through all of them, and rollback will eventually settle on the later entry in the list, so what gets resolved to be the rollback image is UB here.

If the UB here picked up an entry that does have a corresponding /boot BLS entry, this won't create a problem as that entry could then be presumed to be the rollback entry and pass all subsequent checks.
If the UB here picked up a stale entry in /sysroot/state/deploy that does not have a corresponding /boot BLS entry, later here in function set_reboot_capable_type1_deployments:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 444 to 452 in ad60763

	for depl in host
	.status
	.staged
	.iter_mut()
	.chain(host.status.rollback.iter_mut())
	.chain(host.status.other_deployments.iter_mut())
	{
	let entry = find_bls_entry(&depl.require_composefs()?.verity, &bls_entries)?
	.ok_or_else(|| anyhow::anyhow!("Entry not found"))?;

find_bls_entry would fail on host.status.rollback due to the missing BLS entry, causing #1887

As for here in my change, that section is trying to find boot entries that are neither the booted entry nor the staged entry, and are in both /sysroot/state/deploy and /boot. The state that there are two entries satisfying this criteria is currently impossible to be in, since bootc under normal state only keeps one booted entry and one rollback entry in /boot, and under staged state only keeps one booted entry, one old rollback entry and one staged entry in /boot

[Johan-Liebert1]

Thanks for this patch