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

rootless: fix SIGSEGV, Make LISTEN_FDNAMES optional #10481

Merged

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented May 27, 2021

Reproduce SIGSEGV

#!/bin/bash
export LISTEN_PID=$$
export LISTEN_FDS=1
exec podman run --rm registry.fedoraproject.org/fedora:34 /bin/true

[NO TESTS NEEDED]: I think test for this would be a basic run with LISTEN_PID and LISTEN_FDS set and since no new feature or significant behavior change is being added so putting more tests might not be necessary. But sure let me know if having new tests here makes sense.
Closes: #10435

@flouthoc flouthoc force-pushed the fix-sigsegv-rootless branch 2 times, most recently from 1783e56 to f67bb78 Compare May 27, 2021 07:32
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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe

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 May 27, 2021
@rhatdan
Copy link
Member

rhatdan commented May 27, 2021

@giuseppe shouldn't we be saving the ones that are set. LISTEN_FDNAMES is optional, so we should just handle that case.

@giuseppe
Copy link
Member

@giuseppe shouldn't we be saving the ones that are set. LISTEN_FDNAMES is optional, so we should just handle that case.

ah right, yes in that case we need to handle LISTEN_FDNAMES to be null.

@flouthoc could you please fix it in the patch?

@flouthoc
Copy link
Collaborator Author

@rhatdan @giuseppe Sure makes sense, we could make this optional instead.

@flouthoc flouthoc force-pushed the fix-sigsegv-rootless branch 2 times, most recently from 7e8df87 to 7be6483 Compare May 28, 2021 02:22
@flouthoc
Copy link
Collaborator Author

flouthoc commented May 28, 2021

@giuseppe @rhatdan I have made requested changes. Could you please take a look ? I think one failing check is a flake.

@rhatdan
Copy link
Member

rhatdan commented May 28, 2021

LGTM

@eriksjolund
Copy link
Contributor

eriksjolund commented May 28, 2021

Thanks @flouthoc for fixing the issue!

It seems there is no reference in the commit message to the Github issue.

https://github.com/containers/podman/blob/master/CONTRIBUTING.md
PRs that fix issues should include a reference like Closes #XXXX in the commit message so that GitHub will automatically close the referenced issue when the PR is merged.

Another thing, regarding:
"Ref: #10435"
in the comment.
Ref does not seem to be one of the keywords that links a pull request to an issue:
https://docs.github.com/en/issues/tracking-your-work-with-issues/creating-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@flouthoc
Copy link
Collaborator Author

@eriksjolund thanks for pointing this out 😄 .Adding it now Closes: #10435

@flouthoc flouthoc changed the title rootless: fix SIGSEGV, dont call strdup on NULL fdnames rootless: fix SIGSEGV, Make LISTEN_FDNAMES optional May 29, 2021
@flouthoc flouthoc force-pushed the fix-sigsegv-rootless branch 2 times, most recently from 279bd58 to 340e515 Compare May 29, 2021 02:54
@eriksjolund
Copy link
Contributor

Thanks @flouthoc Could you also add

Closes #10435

in the commit message? (The information will then be preserved in the commit log. Another advantage is that it will help GitHub provide a convenient hyperlink to the issue when displaying the Git commit on their website)

Running git log I see that there are also other styles that works as well, for instance

Closes: https://github.com/containers/podman/issues/10435

@flouthoc flouthoc force-pushed the fix-sigsegv-rootless branch 2 times, most recently from 5052221 to b7e304b Compare May 29, 2021 08:09
@flouthoc flouthoc force-pushed the fix-sigsegv-rootless branch from b7e304b to 2addc0f Compare May 29, 2021 09:47
@flouthoc
Copy link
Collaborator Author

@eriksjolund Done. Let me know if anything else needs to be done. 🙂

@eriksjolund
Copy link
Contributor

@flouthoc Thanks, to me it looks fine. I now see the GitHub hyperlink in flouthoc@2addc0f

@rhatdan
Copy link
Member

rhatdan commented May 30, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit 5923676 into containers:master May 30, 2021
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

segfault when LISTEN_PID and LISTEN_FDS are set but not LISTEN_FDNAMES
5 participants