Skip to content

Prevent NPE in addServiceInfoToCluster() #2239

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.

Sign up for GitHub

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

Closed
thaJeztah wants to merge 1 commit into from
Closed

Prevent NPE in addServiceInfoToCluster() #2239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ func (ep *endpoint) deleteDriverInfoFromCluster() error {
}

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 {
Copy link
Member Author

@thaJeztah thaJeztah Jul 20, 2018

Choose a reason for hiding this comment

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

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.

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion endpoint_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func (ep *endpoint) Iface() InterfaceInfo {
return ep.iface
}

return nil
return &endpointInterface{}
Copy link
Member Author

@thaJeztah thaJeztah Jul 20, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ctelfer ctelfer Jul 27, 2018

Choose a reason for hiding this comment

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

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.

}

func (ep *endpoint) Interface() driverapi.InterfaceInfo {
Expand Down