-
Notifications
You must be signed in to change notification settings - Fork 385
container: fix error release #1933
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
Conversation
Signed-off-by: Erik Sjölund <[email protected]>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsure container deletion path correctly releases the main error object before delegating to libcrun_container_delete_status, avoiding an error leak in the non-OCI-state-root branch. File-Level Changes
Possibly linked issuesTips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The new
crun_error_release(err);appears to freeerrbefore it is passed tolibcrun_container_delete_status(state_root, id, err), which risks a use-after-free; consider either moving the release after the call or adjustinglibcrun_container_delete_statususage so you are not passing a released error object.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `crun_error_release(err);` appears to free `err` before it is passed to `libcrun_container_delete_status(state_root, id, err)`, which risks a use-after-free; consider either moving the release after the call or adjusting `libcrun_container_delete_status` usage so you are not passing a released error object.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
TMT tests failed. @containers/packit-build please check. |
giuseppe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by Sourcery
Bug Fixes: