Skip to content

Comments

Check for nil before using HostConfig to adjustCpuShares#14917

Merged
LK4D4 merged 1 commit intomoby:masterfrom
srust:14915-empty-hostconfig-on-create
Jul 27, 2015
Merged

Check for nil before using HostConfig to adjustCpuShares#14917
LK4D4 merged 1 commit intomoby:masterfrom
srust:14915-empty-hostconfig-on-create

Conversation

@srust
Copy link
Contributor

@srust srust commented Jul 23, 2015

Hi,

This fixes #14915 for me.

The remote API in master does not work for a /container/create request that comes in with a missing HostConfig sub-object. Docker will dereference a nil pointer.

The 'adjustCpuShares' function expects a valid HostConfig object.

It appears that the commit for #13218 changed the behavior of the decoder, to no longer create a new HostConfig object if none was specified over the API. That change appears to have broken the create call. With this fix applied, the issue no longer occurs.

Thanks!

Signed-off-by: Stephen Rust [email protected]

@runcom
Copy link
Member

runcom commented Jul 23, 2015

@srust thanks! Can you add a failing test case for this change also? And put the HostConfig nil check inside AdjustCpuShares pls

@runcom runcom added this to the 1.8.0 milestone Jul 25, 2015
@runcom
Copy link
Member

runcom commented Jul 25, 2015

this is a bug where when the client calls an older api (specifically < 1.19) version w/o hostconfig it results in this nil pointer dereference, failing test case:

diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go
index b0e9b0e..48b4fc8 100644
--- a/integration-cli/docker_api_containers_test.go
+++ b/integration-cli/docker_api_containers_test.go
@@ -1687,3 +1687,13 @@ func (s *DockerSuite) TestPostContainersStartWithLinksInHostConfigIdLinked(c *ch
        c.Assert(res.StatusCode, check.Equals, http.StatusNoContent)
        b.Close()
 }
+
+// #14915
+func (s *DockerSuite) TestPostContainersCreateWithoutHostConfigOnOldAPI(c *check.C) {
+       config := struct {
+               Image string
+       }{"busybox"}
+       status, _, err := sockRequest("POST", "/v1.18/containers/create", config)
+       c.Assert(err, check.IsNil)
+       c.Assert(status, check.Equals, http.StatusCreated)
+}

ping @calavera I've added this to 1.8 (remove it if I've mistaken)

@srust
Copy link
Contributor Author

srust commented Jul 25, 2015

Ah thanks @runcom for the test case! Was just working through the test suite to add this. I'll move the nil check.

@srust srust force-pushed the 14915-empty-hostconfig-on-create branch from a3e9b0e to 5e875be Compare July 25, 2015 11:45
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 25, 2015
@srust srust force-pushed the 14915-empty-hostconfig-on-create branch from 5e875be to 5ebd73a Compare July 25, 2015 11:45
@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jul 25, 2015
@srust srust force-pushed the 14915-empty-hostconfig-on-create branch from e4dae9d to 0933fa4 Compare July 25, 2015 12:47
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 25, 2015
@runcom
Copy link
Member

runcom commented Jul 25, 2015

@srust could you also squash your commits down to just one pls?

@srust
Copy link
Contributor Author

srust commented Jul 25, 2015

@runcom I've commited your patch for the unit test (hope that's okay). thanks again. I've pushed both the fix and test case and rebased with master. Let me know anything else you need for this. Thanks!

@srust srust force-pushed the 14915-empty-hostconfig-on-create branch from 0933fa4 to 3c73b1a Compare July 25, 2015 12:51
@srust
Copy link
Contributor Author

srust commented Jul 25, 2015

Squashed to one commit.

@runcom
Copy link
Member

runcom commented Jul 25, 2015

LGTM

cpuguy83
cpuguy83 reviewed Jul 26, 2015
View reviewed changes
integration-cli/docker_api_containers_test.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Jul 26, 2015

Choose a reason for hiding this comment

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

TestContainersApiCreateNoHostConfig118

Check for nil before using HostConfig to adjustCpuShares
c358a4c 
Fix moby#14915. Add unit test for moby#14915.
Thanks @runcom for the test case: when the client calls 1.18 api
version w/o hostconfig it results in a nil pointer dereference.

Signed-off-by: Stephen Rust <[email protected]>
@srust srust force-pushed the 14915-empty-hostconfig-on-create branch from 3c73b1a to c358a4c Compare July 27, 2015 00:46
@srust
Copy link
Contributor Author

srust commented Jul 27, 2015

Renamed unit test from code review.

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 27, 2015

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Jul 27, 2015

LGTM

LK4D4 added a commit that referenced this pull request Jul 27, 2015
@LK4D4
Merge pull request #14917 from srust/14915-empty-hostconfig-on-create
ec8173b 
Check for nil before using HostConfig to adjustCpuShares
@LK4D4 LK4D4 merged commit ec8173b into moby:master Jul 27, 2015
@srust srust mentioned this pull request Jul 27, 2015
Closed
@calavera
Copy link
Contributor

calavera commented Jul 30, 2015

cherry picked into #15091

@thaJeztah thaJeztah mentioned this pull request Jan 22, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

1.8.0

Development

Successfully merging this pull request may close these issues.

remote API container create nil pointer dereference if HostConfig is not specified

6 participants

@srust @runcom @cpuguy83 @LK4D4 @calavera @GordonTheTurtle