Prevent NPE in addServiceInfoToCluster() #2239
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`ep.Iface()` can return `nil`, in which case `Address()` would result in a NPE. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
commented
Jul 20, 2018
|
|
||
| func (ep *endpoint) addServiceInfoToCluster(sb *sandbox) error { | ||
| if ep.isAnonymous() && len(ep.myAliases) == 0 || ep.Iface().Address() == nil { | ||
| if ep.isAnonymous() && len(ep.myAliases) == 0 || ep.Iface() == nil || ep.Iface().Address() == nil { |
Member
Author
There was a problem hiding this comment.
Could use some input here; I started with this, but noticed there's quite some places where a "fluent" interface is used (foo.Iface().SomeFunction()), so having to check in all places is likely problematic.
thaJeztah
commented
Jul 20, 2018
| } | ||
|
|
||
| return nil | ||
| return &endpointInterface{} |
Member
Author
There was a problem hiding this comment.
Perhaps this would work to prevent the situation, but I see there's many other places where nil can be returned, which won't really work with a fluent interface 😓, so likely all those occurrences need to be addressed?
Contributor
There was a problem hiding this comment.
Ok, so I grepped the code specifically for \.Iface\(\) and came away with the following locations where it is used. Most of the fluid uses are protected by tests and so don't need further protection. Here are the locations I think need further protection:
- agent.go L586 -- already mentioned above
- agent.go L709 -- could add a protection similar to L586 at the start of the function: L668
- controller.go L955 -- Probably needs an
if ep.Iface() == nil { continue }right before - network.go L1317 -- Should probably become
iface := ep.Iface(); if iface != nil && iface.Address() != nil { ...
So we should be covered without returning the&endpointInterface{}if we add three more tests.
Just as a side comment:
- service_windows.go L41 -- should be protected by epInfo.LoadBalancer() test
- service_linux.go (all) -- all fluid references are protected by a call to findLBEndpointSandbox() which only returns the endpoint if
epi.LoadBalancer()is true. (as per Windows above) - maybe there's some kind of race in those files where somone tries to add a binding while the sandbox is still coming up? But I'm not sure. If so, then a test in findLBEndpointSandbox() would be in order for
service_linux.go. - there are a few test files that are unprotected -- I'm not worried about those.
I'm not really sure why we can have endpoints without interfaces -- I thought these were a 1-to-1 mapping conceptually. But given comments in service_linux.go that indicate that this is deliberate, I'm disinclined to return a pointer to an empty structure. OTOH, I don't see anywhere in the code that is positively acting on Iface() returning nil. So, maybe it is OK after all w/ a small change to L36 of service_linux.go. Others w/ more history, please chime in.
Member
Author
|
ping @fcrisciani @ctelfer |
Contributor
|
I see blow files are having similar implementation and invokes "ep.Iface().Address()" without checking if Iface returns null or not. @fcrisciani WDYT? |
|
Just fiy, this is still happening with docker 18.09 |
Contributor
|
@selansen is this still needed ? |
|
@arkodg I just ran into this same bug, so it's likely still needed docker info: Panic trace: |
arkodg
pushed a commit
to arkodg/libnetwork
that referenced
this pull request
Apr 3, 2020
This PR carryforwards moby#2239 and incorporates the suggestions in comments to fix the NPE and potential NPEs due to null value returned by ep.Iface() Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
arkodg
pushed a commit
to arkodg/libnetwork
that referenced
this pull request
Apr 3, 2020
This PR carryforwards moby#2239 and incorporates the suggestions in comments to fix the NPE and potential NPEs due to null value returned by ep.Iface() Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
arkodg
pushed a commit
to arkodg/libnetwork
that referenced
this pull request
Apr 3, 2020
This PR carryforwards moby#2239 and incorporates the suggestions in comments to fix the NPE and potential NPEs due to a null value returned by ep.Iface() Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Apr 21, 2020
full diff: moby/libnetwork@ef149a9...1a17fb3 - moby/libnetwork#2538 produce an error with invalid address pool - addresses moby#40388 dockerd ignores the --default-address-pool option - moby/libnetwork#2471 DOCKER-USER chain not created when IPTableEnable=false - moby/libnetwork#2544 Fix NPE due to null value returned by ep.Iface() - carries moby/libnetwork#2239 Prevent NPE in addServiceInfoToCluster() - addresses moby#37506 Error initializing docker.server while starting daemon by systemd Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Apr 25, 2020
full diff: moby/libnetwork@ef149a9...1a17fb3 - moby/libnetwork#2538 produce an error with invalid address pool - addresses moby/moby#40388 dockerd ignores the --default-address-pool option - moby/libnetwork#2471 DOCKER-USER chain not created when IPTableEnable=false - moby/libnetwork#2544 Fix NPE due to null value returned by ep.Iface() - carries moby/libnetwork#2239 Prevent NPE in addServiceInfoToCluster() - addresses moby/moby#37506 Error initializing docker.server while starting daemon by systemd Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: c3808634e7a44fc5b4fb8caacd9d079f7e0a0fee Component: engine
cpuguy83
pushed a commit
to cpuguy83/docker
that referenced
this pull request
May 25, 2021
This PR carryforwards moby/libnetwork#2239 and incorporates the suggestions in comments to fix the NPE and potential NPEs due to a null value returned by ep.Iface() Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ep.Iface()can returnnil, in which caseAddress()would result in a NPE.relates to moby/moby#37506