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

reexec: drop Pdeathsig #530

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Conversation

giuseppe
Copy link
Member

it can lead to a race condition where the process is killed even if
the parent process is still running.

It can happen as the parent in the prctl(PR_SET_PDEATHSIG) case is
considered the thread that created the process. In the case of the
Go runtime the thread could be freed by the runtime at any time
causing the re-execed process to receive the SIGTERM.

Signed-off-by: Giuseppe Scrivano [email protected]

it can lead to a race condition where the process is killed even if
the parent process is still running.

It can happen as the parent in the prctl(PR_SET_PDEATHSIG) case is
considered the *thread* that created the process.  In the case of the
Go runtime the thread could be freed by the runtime at any time
causing the re-execed process to receive the SIGTERM.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe requested review from nalind and rhatdan and removed request for nalind February 13, 2020 09:14
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

@giuseppe Do we loose any functionality from removing Pdeathsig?

@giuseppe
Copy link
Member Author

@giuseppe Do we loose any functionality from removing Pdeathsig?

yes the re-exec'ed process won't automatically get a SIGTERM when the parent process exits, but I think that is a better behaviour than getting spurious signals

Before we merge, let's wait for @nalind, I'd like his review on this one

@nalind
Copy link
Member

nalind commented Feb 13, 2020

Nice catch! LGTM.

@TomSweeneyRedHat
Copy link
Member

LGTM
this needs one more approved review and I thought we would have had this by now with the reviews already.

@rhatdan rhatdan merged commit 9b53144 into containers:master Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants