The Wayback Machine - https://web.archive.org/web/20260314083428/https://github.com/docker/cli/pull/303
Skip to content

fixing SIGSEGV when running containers#303

Merged
aaronlehmann merged 1 commit intodocker:masterfrom
yastij:fix-SIGSEGV-runContainer
Jul 6, 2017
Merged

fixing SIGSEGV when running containers#303
aaronlehmann merged 1 commit intodocker:masterfrom
yastij:fix-SIGSEGV-runContainer

Conversation

@yastij
Copy link
Contributor

@yastij yastij commented Jul 6, 2017

Signed-off-by: Yassine TIJANI yasstij11@gmail.com

- How I did it checking that close is not before deferring it.

fixes moby/moby#33975

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #303 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #303   +/-   ##
=======================================
  Coverage   48.68%   48.68%           
=======================================
  Files         185      185           
  Lines       12168    12168           
=======================================
  Hits         5924     5924           
  Misses       5877     5877           
  Partials      367      367
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!


if close != nil {
defer close()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more common way of doing this is to just move the defer close() after the if err != nil. This is only nil if there is an error, so handling the error first will fix the problem.

Copy link
Contributor Author

@yastij yastij Jul 6, 2017

Choose a reason for hiding this comment

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

@dnephin - indeed you are right. i thought that we might have no error and still have nil close.

Thanks for the hint.

@yastij
Copy link
Contributor Author

yastij commented Jul 6, 2017

@dnephin - LGTY ?

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Please also squash to a single git commit.

Signed-off-by: Yassine TIJANI <yasstij11@gmail.com>

moving the deffering of the close after the error checking

Signed-off-by: Yassine TIJANI <yasstij11@gmail.com>

fixing SIGSEGV when running containers

Signed-off-by: Yassine TIJANI <yasstij11@gmail.com>
@yastij yastij force-pushed the fix-SIGSEGV-runContainer branch from 384fa20 to 45b0e7c Compare July 6, 2017 16:58
@yastij
Copy link
Contributor Author

yastij commented Jul 6, 2017

@dnephin - Done.

@aaronlehmann
Copy link

LGTM

@aaronlehmann aaronlehmann merged commit dfdbdab into docker:master Jul 6, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jul 6, 2017
@yastij yastij deleted the fix-SIGSEGV-runContainer branch July 20, 2017 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment