Skip to content
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

server: close net.Conn unconditionally after accepting #106072

Merged
merged 1 commit into from
Jul 7, 2023
Merged
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions pkg/server/server_controller_sql.go
Original file line number Diff line number Diff line change
@@ -50,6 +50,10 @@ func (c *serverController) sqlMux(
// The concurrent dispatch gives a chance to the succeeding
// servers to see and process the cancel at approximately the
// same time as every other.
//
// Cancel requests are unauthenticated so run the cancel async to prevent
// the client from deriving any info about the cancel based on how long it
// takes.
if err := c.stopper.RunAsyncTask(ctx, "cancel", func(ctx context.Context) {
s.handleCancel(ctx, status.CancelKey)
}); err != nil {
8 changes: 6 additions & 2 deletions pkg/server/tenant.go
Original file line number Diff line number Diff line change
@@ -833,8 +833,12 @@ func (s *SQLServerWrapper) serveConn(
pgServer := s.PGServer()
switch status.State {
case pgwire.PreServeCancel:
pgServer.HandleCancel(ctx, status.CancelKey)
return nil
// Cancel requests are unauthenticated so run the cancel async to prevent
// the client from deriving any info about the cancel based on how long it
// takes.
return s.stopper.RunAsyncTask(ctx, "cancel", func(ctx context.Context) {
pgServer.HandleCancel(ctx, status.CancelKey)
})
case pgwire.PreServeReady:
return pgServer.ServeConn(ctx, conn, status)
default:
6 changes: 2 additions & 4 deletions pkg/sql/pgwire/pre_serve.go
Original file line number Diff line number Diff line change
@@ -217,7 +217,6 @@ func (s *PreServeConnHandler) SendRoutingError(
`Double check your "-ccluster=" connection option or your "cluster:" database name prefix.`)

_ = s.sendErr(ctx, s.st, conn, err)
_ = conn.Close()
}

// sendErr sends errors to the client during the connection startup
@@ -235,7 +234,6 @@ func (s *PreServeConnHandler) sendErr(
// receive error payload are highly correlated with clients
// disconnecting abruptly.
_ /* err */ = w.writeErr(ctx, err, conn)
_ = conn.Close()
return err
}

@@ -324,7 +322,7 @@ func (s *PreServeConnHandler) PreServe(
case versionCancel:
// The cancel message is rather peculiar: it is sent without
// authentication, always over an unencrypted channel.
if ok, key := readCancelKeyAndCloseConn(ctx, conn, &buf); ok {
if ok, key := readCancelKey(ctx, &buf); ok {
return conn, PreServeStatus{
State: PreServeCancel,
CancelKey: key,
@@ -374,7 +372,7 @@ func (s *PreServeConnHandler) PreServe(
// Yet, we've found clients in the wild that send the cancel
// after the TLS handshake, for example at
// https://github.com/cockroachlabs/support/issues/600.
if ok, key := readCancelKeyAndCloseConn(ctx, conn, &buf); ok {
if ok, key := readCancelKey(ctx, &buf); ok {
return conn, PreServeStatus{
State: PreServeCancel,
CancelKey: key,
14 changes: 3 additions & 11 deletions pkg/sql/pgwire/server.go
Original file line number Diff line number Diff line change
@@ -928,8 +928,6 @@ func (s *Server) serveImpl(
authOpt authOptions,
sessionID clusterunique.ID,
) {
defer func() { _ = c.conn.Close() }()

if c.sessionArgs.User.IsRootUser() || c.sessionArgs.User.IsNodeUser() {
ctx = logtags.AddTag(ctx, "user", redact.Safe(c.sessionArgs.User))
} else {
@@ -1253,18 +1251,13 @@ func (s *Server) serveImpl(
}
}

// readCancelKeyAndCloseConn retrieves the "backend data" key that identifies
// readCancelKey retrieves the "backend data" key that identifies
// a cancellable query, then closes the connection.
func readCancelKeyAndCloseConn(
ctx context.Context, conn net.Conn, buf *pgwirebase.ReadBuffer,
func readCancelKey(
ctx context.Context, buf *pgwirebase.ReadBuffer,
) (ok bool, cancelKey pgwirecancel.BackendKeyData) {
telemetry.Inc(sqltelemetry.CancelRequestCounter)
backendKeyDataBits, err := buf.GetUint64()
// The connection that issued the cancel is not a SQL session -- it's an
// entirely new connection that's created just to send the cancel. We close
// the connection as soon as possible after reading the data, since there
// is nothing to send back to the client.
_ = conn.Close()
// The client is also unwilling to read an error payload, so we just log it locally.
if err != nil {
log.Sessions.Warningf(ctx, "%v", errors.Wrap(err, "reading cancel key from client"))
@@ -1378,7 +1371,6 @@ func (s *Server) sendErr(
// receive error payload are highly correlated with clients
// disconnecting abruptly.
_ /* err */ = w.writeErr(ctx, err, conn)
_ = conn.Close()
return err
}

3 changes: 3 additions & 0 deletions pkg/util/netutil/net.go
Original file line number Diff line number Diff line change
@@ -183,6 +183,9 @@ func (s *TCPServer) ServeWith(
}
tempDelay = 0
err := s.stopper.RunAsyncTask(ctx, "tcp-serve", func(ctx context.Context) {
defer func() {
_ = rw.Close()
}()
s.addConn(rw)
defer s.rmConn(rw)
serveConn(ctx, rw)