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.
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
[FIXED] Possible panic when server accepts TLS leafnode connection #1652
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this solve for the panic though and the stack trace we saw in Slack?
@derekcollison The panic was for the non solicited. When calling |
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
56dd752
to
3b8d00e
Compare
Pushing the change for the wrong comment.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, maybe make that a small internal function inside the create function.
server/leafnode.go
Outdated
@@ -649,6 +649,14 @@ func (s *Server) createLeafNode(conn net.Conn, remote *leafNodeCfg) *client { | |||
return nil | |||
} | |||
|
|||
// Not sure that can happen, but in case the connection was marked | |||
// as closed during the call to parse... | |||
if c.isClosed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a small anonymous function inside this function to clean up?
ifClosedReturn := func()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still need to have something like:
if closedReturn() {
return nil
}
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is an addition to PR #1652. I have simply added a check but at this point in time there is no risk that connection is closed this early. I also renamed the small helper function and fixed a test that had an improper `s.mu.Unlock()` in an error condition. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This is a port of PR #1652/#1660. The code in the 2.1.x branch is not sensitive to the issue fixed in these PRs because marking the connection as closed (for instance due to a TCP error in sendProtoNow) will not set `c.nc` to nil, so there won't be the nil dereference issue that was found in the main branch. However, porting the code for extra safety. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Signed-off-by: Ivan Kozlovic ivan@synadia.com