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

Change to correct break statements to prevent infinite recursion causing OOM #16166

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

misuto
Copy link
Contributor

@misuto misuto commented Oct 13, 2022

Problem description and Solution

I discovered that an image pull could end up in infinite recursion if the context is cancelled while pulling. The reason is that the break statement only breaks the select instead of the for.
I found a few other places in pkg/bindings that also seemed to have the same issue, which I included a fix for. The solution is to put a label at the for loop and reference that when breaking in the cases for pull.go and push.go. In manifests.go there were no return statement outside of the for loop so I opted to return an empty string along with the error context.Canceled.

Does this PR introduce a user-facing change?

Prevent OOM when context is cancelled in a few situations 

[NO NEW TESTS NEEDED]

@misuto
Copy link
Contributor Author

misuto commented Oct 13, 2022

There does not seem to be any fitting test I can modify or extend. Am I missing something or is it not implemented yet?
https://github.com/containers/podman/blob/main/test/system/TODO.md seems to indicate that tests pull is not implemented yet, but I don't know if the bindings package are included in this.

Should I skip writing tests?

@edsantiago
Copy link
Member

Thanks for your PR and for asking about tests. https://github.com/containers/podman/blob/24b586e7d6b7b4940aef8a5929283519020d2806/test/system/330-corrupt-images.bats might be a candidate for a test; but from my quick reading, is there even a reliable reproducer for this?

@misuto
Copy link
Contributor Author

misuto commented Oct 13, 2022

Thanks for your PR and for asking about tests. https://github.com/containers/podman/blob/24b586e7d6b7b4940aef8a5929283519020d2806/test/system/330-corrupt-images.bats might be a candidate for a test; but from my quick reading, is there even a reliable reproducer for this?

Thanks for your reply!
When I discovered this I did it purely by accident in the beginning and it took a few tries to figure out what the problem was.
The very hacky reproduction steps are something like this:

  1. Create new context with cancel
  2. Create podman connection with NewConnection, with the ctx created in previous step
  3. Start image pull in go routine
  4. Wait one second and call cancel in the context created in step 1

That seemed to reproduce the issue alright, but it's not a pretty solution. The faults with selects in push.go and manifest.go was found with the staticcheck tool so I guess that is one way to test that those types of issues won't make it back into the code base again.

@rhatdan rhatdan requested a review from vrothberg October 14, 2022 14:04
@rhatdan
Copy link
Member

rhatdan commented Oct 14, 2022

@mheon PTAL

@mheon
Copy link
Member

mheon commented Oct 14, 2022

Code LGTM over here.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, misuto

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2022
@rhatdan
Copy link
Member

rhatdan commented Oct 14, 2022

@misuto Could you rebase and repush, so the tests will pass.

Signed-off-by: Jakob Tigerström <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2022

Awesome work @misuto Thanks.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2022
@openshift-merge-robot openshift-merge-robot merged commit d21a356 into containers:main Oct 15, 2022
@misuto
Copy link
Contributor Author

misuto commented Oct 15, 2022

Awesome work @misuto Thanks. /lgtm

Thank you, hope I can contribute with something more in the future!

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants