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

store: correctly remove incomplete layers on load. #2185

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Dec 3, 2024

In go one should never modify a slice while also iterating over it at the same time. This causes weird side effects as the underlying array elements are shifted around without the range loop index knowing. So if you delete a element the loop will then actually skip the next one and theoretically access out of bounds on the last element which does not panic but rather return the default zero type, nil here which then causes the panic on layer.Flags == nil.

Here is a simple example to show the behavior:

func main() {
	slice := []int{1, 2, 3, 4, 5, 6, 7, 8, 9}
	for _, num := range slice {
		if num == 5 {
			slice = slices.DeleteFunc(slice, func(n int) bool {
				return n == 5
			})
		}
		fmt.Println(num)
	}
}

The loop will not print 6, but then as last number it prints 0 (the default zero type for an int).

Fixes #2184

@Luap99
Copy link
Member Author

Luap99 commented Dec 3, 2024

@giuseppe @mtrmac PTAL

Untested at this point as I am not familiar with the storage layout so I am not sure how I can corrupt my storage in such case to trigger this code path.

I have also not yet audited other code paths for a similar issues. I wonder if there is a linter for this thing.

Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't resist noting this type of bug can't happen in Rust.

layers.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great find!

layers.go Outdated Show resolved Hide resolved
layers.go Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator

mtrmac commented Dec 3, 2024

Untested at this point as I am not familiar with the storage layout so I am not sure how I can corrupt my storage in such case to trigger this code path.

In this case, probably by deleting an image (or a single layer): that marks layers as incomplete and proceeds to delete the layer files. If the deletion is aborted (by panic / reboot / SIGKILL) at that time, the layer remains as incomplete and triggers this late deletion. (It would also be necessary to arrange that the layer being deleted is not the last one in r.layers, or the loop would terminate.)

I don’t think there is a way to write a reliable test for this (using the stable API); the caller would have to trigger deletion of a layer in one thread, and abort the process in another, hoping that it aborts it during the filesystem deletion. (Alternatively, a hack would be to call SetFlag with the incomplete flag explicitly; that should not be too hard to trigger, but it’s not a public API, and it seems to me like a rather specific scenario to write a test for.)

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 3, 2024

/approve

layers.go Outdated Show resolved Hide resolved
In go one should never modify a slice while also iterating over it at
the same time. This causes weird side effects as the underlying array
elements are shifted around without the range loop index knowing.
So if you delete a element the loop will then actually skip the next one
and theoretically access out of bounds on the last element which does
not panic but rather return the default zero type, nil here which then
causes the panic on layer.Flags == nil.

Here is a simple example to show the behavior:
func main() {
	slice := []int{1, 2, 3, 4, 5, 6, 7, 8, 9}
	for _, num := range slice {
		if num == 5 {
			slice = slices.DeleteFunc(slice, func(n int) bool {
				return n == 5
			})
		}
		fmt.Println(num)
	}
}

The loop will not print 6, but then as last number it prints 0 (the
default zero type for an int).

Fixes containers#2184

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

openshift-ci bot commented Dec 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe, Luap99, mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

Successfully merging this pull request may close these issues.

Can't run any podman command anymore - panic: runtime error: invalid memory address or nil pointer dereference
4 participants