-
Notifications
You must be signed in to change notification settings - Fork 378
container: fix error release #1918
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 GuideAdjusts error handling in setup_executable_path so that non-ENOENT errors are released only when a specific condition is met, preventing premature error release in the container setup path. File-Level Changes
Possibly linked issues
Tips 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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/container.c:1238-1242` </location>
<code_context>
return ret;
- }
- /* If it fails for any other reason, ignore the failure. We'll try again the lookup
- once the process switched to the use that runs in the container. This might be necessary
- when opening a file that is on a network file system like NFS, where CAP_DAC_OVERRIDE
- is not honored. */
- crun_error_release (err);
+ /* If it fails for any other reason, ignore the failure. We'll try again the lookup
+ once the process switched to the use that runs in the container. This might be necessary
</code_context>
<issue_to_address>
**issue (bug_risk):** The error object is never released anymore and the comment no longer matches the behavior.
With `crun_error_release(err);` moved inside the `if` and after `return ret;`, it becomes unreachable for the ENOENT path and is never called for other errors (since there is no release after the `if`). This changes behavior from "ignore failure but clean up" to leaking `err`. Please move `crun_error_release(err);` back outside the `if` or place it before the `return` so all paths release the error while keeping the ignore-and-retry behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| once the process switched to the use that runs in the container. This might be necessary | ||
| when opening a file that is on a network file system like NFS, where CAP_DAC_OVERRIDE | ||
| is not honored. */ | ||
| crun_error_release (err); | ||
| } |
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.
issue (bug_risk): The error object is never released anymore and the comment no longer matches the behavior.
With crun_error_release(err); moved inside the if and after return ret;, it becomes unreachable for the ENOENT path and is never called for other errors (since there is no release after the if). This changes behavior from "ignore failure but clean up" to leaking err. Please move crun_error_release(err); back outside the if or place it before the return so all paths release the error while keeping the ignore-and-retry behavior.
|
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: