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

Jump to bottom

Fix exec race condition #2970

Merged
merged 2 commits into from Feb 1, 2019
Merged

Fix exec race condition #2970

merged 2 commits into from Feb 1, 2019

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Feb 1, 2019 •
edited

Fixes #2969.

No one likes nested locks. :( I know.

I'm happy to change the fix if people come up with better ideas.

/cc @containerd/containerd-maintainers @containerd/containerd-reviewers

@Random-Liu
Revert "use state machine management for exec.Pid()"
9777d76 
This reverts commit bbc2a99.

Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Copy link
Member Author

We should cherry-pick this into 1.2 and cut a 1.2.3 release soon. I think this is a very serious issue, which will affect both docker users and kubernetes users.

@ehazlett
Copy link
Member

ehazlett commented Feb 1, 2019 •
edited

This is having some build and lint issues:

# github.com/containerd/containerd/runtime/v1/linux/proc
runtime/v1/linux/proc/init.go:410:15: type safePid is not an expression
make: *** [build] Error 2
The command "make build" exited with 2.

@Random-Liu
Add a separate lock for pid.
952d582 
Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Copy link
Member Author

@ehazlett Sorry, fixed.

@codecov-io
Copy link

Codecov Report

Merging #2970 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2970      +/-   ##
=========================================
+ Coverage   43.98%     44%   +0.02%     
=========================================
  Files         102     102              
  Lines       10873   10873              
=========================================
+ Hits         4782    4785       +3     
+ Misses       5358    5356       -2     
+ Partials      733     732       -1
Flag Coverage Δ
#linux 47.63% <ø> (ø) ⬆️
#windows 41.22% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
gc/scheduler/scheduler.go 68.75% <0%> (+1.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7f24e2...952d582. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #2970 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2970      +/-   ##
=========================================
+ Coverage   43.98%     44%   +0.02%     
=========================================
  Files         102     102              
  Lines       10873   10873              
=========================================
+ Hits         4782    4785       +3     
+ Misses       5358    5356       -2     
+ Partials      733     732       -1
Flag Coverage Δ
#linux 47.63% <ø> (ø) ⬆️
#windows 41.22% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
gc/scheduler/scheduler.go 68.75% <0%> (+1.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7f24e2...952d582. Read the comment docs.

@crosbymichael
Copy link
Member

LGTM

cpuguy83
cpuguy83 reviewed Feb 1, 2019
View reviewed changes
runtime/v1/linux/proc/exec.go
// In that case, we want to block the signal handler to
// access e.pid until it is updated.
e.pid.Lock()
defer e.pid.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

Check if the context is cancelled here?

@ehazlett
Copy link
Member

ehazlett commented Feb 1, 2019

LGTM

@crosbymichael crosbymichael merged commit 6b25c1e into containerd:master Feb 1, 2019
@crosbymichael
Copy link
Member

Lets do the cherry pick and do a release next week, maybe Wednesday? @dmcgowan ??

@Random-Liu
Copy link
Member Author

Random-Liu commented Feb 1, 2019 via email

@crosbymichael
Copy link
Member

Yup, i'll cherry pick right now

@crosbymichael crosbymichael mentioned this pull request Feb 1, 2019
Merged
@Random-Liu Random-Liu deleted the fix-exec-race-condition branch February 2, 2019 00:05
@thaJeztah thaJeztah mentioned this pull request Feb 12, 2019
Merged
@nirmaldavis nirmaldavis mentioned this pull request May 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x78 pc=0x5ff488]
6 participants
@Random-Liu @ehazlett @codecov-io @crosbymichael @cpuguy83 @AkihiroSuda