Skip to content

Fix exec race condition#2970

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
Random-Liu:fix-exec-race-condition
Feb 1, 2019
Merged

Fix exec race condition#2970
crosbymichael merged 2 commits intocontainerd:masterfrom
Random-Liu:fix-exec-race-condition

Conversation

@Random-Liu
Copy link
Copy Markdown
Member

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

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 <[email protected]>
@Random-Liu
Copy link
Copy Markdown
Member Author

Random-Liu commented Feb 1, 2019

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
Copy Markdown
Member

ehazlett commented Feb 1, 2019 •
edited
Loading

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 Random-Liu force-pushed the fix-exec-race-condition branch from 473ae35 to 952d582 Compare February 1, 2019 16:48
@Random-Liu
Copy link
Copy Markdown
Member Author

Random-Liu commented Feb 1, 2019

@ehazlett Sorry, fixed.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 1, 2019

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
Copy Markdown

codecov-io commented Feb 1, 2019

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
Copy Markdown
Member

crosbymichael commented Feb 1, 2019

LGTM

cpuguy83
cpuguy83 reviewed Feb 1, 2019
View reviewed changes
Comment thread 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
Copy Markdown
Member

@cpuguy83 cpuguy83 Feb 1, 2019

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
Copy Markdown
Member

ehazlett commented Feb 1, 2019

LGTM

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

crosbymichael commented Feb 1, 2019

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

@Random-Liu
Copy link
Copy Markdown
Member Author

Random-Liu commented Feb 1, 2019 via email

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 1, 2019

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