tm2/pkg/bft/node: Node.startRPC should be strict and close all other listeners on any error instead of leaking them

[odeke-em]

On any error tm2/pkg/bft/node/Node.startRPC leaks all the other created listeners without closing, which can cause simply an irritation for folks trying to restart a node in case the listeners aren't released by the OS directly. This is not a vulnerability but a coding error.

If we examine this code

gno/tm2/pkg/bft/node/node.go

Lines 716 to 792 in 15d119f

	func (n *Node) startRPC() ([]net.Listener, error) {
	listenAddrs := splitAndTrimEmpty(n.config.RPC.ListenAddress, ",", " ")
	config := rpcserver.DefaultConfig()
	config.MaxBodyBytes = n.config.RPC.MaxBodyBytes
	config.MaxHeaderBytes = n.config.RPC.MaxHeaderBytes
	config.MaxOpenConnections = n.config.RPC.MaxOpenConnections
	// If necessary adjust global WriteTimeout to ensure it's greater than
	// TimeoutBroadcastTxCommit.
	// See https://github.com/gnolang/gno/tm2/pkg/bft/issues/3435
	if config.WriteTimeout <= n.config.RPC.TimeoutBroadcastTxCommit {
	config.WriteTimeout = n.config.RPC.TimeoutBroadcastTxCommit + 1*time.Second
	}
	// we may expose the rpc over both a unix and tcp socket
	var rebuildAddresses bool
	listeners := make([]net.Listener, len(listenAddrs))
	for i, listenAddr := range listenAddrs {
	mux := http.NewServeMux()
	rpcLogger := n.Logger.With("module", "rpc-server")
	wmLogger := rpcLogger.With("protocol", "websocket")
	wm := rpcserver.NewWebsocketManager(rpccore.Routes,
	rpcserver.OnDisconnect(func(remoteAddr string) {
	// any cleanup...
	// (we used to unsubscribe from all event subscriptions)
	}),
	rpcserver.ReadLimit(config.MaxBodyBytes),
	)
	wm.SetLogger(wmLogger)
	mux.HandleFunc("/websocket", wm.WebsocketHandler)
	rpcserver.RegisterRPCFuncs(mux, rpccore.Routes, rpcLogger)
	if strings.HasPrefix(listenAddr, "tcp://") && strings.HasSuffix(listenAddr, ":0") {
	rebuildAddresses = true
	}
	listener, err := rpcserver.Listen(
	listenAddr,
	config,
	)
	if err != nil {
	return nil, err
	}
	var rootHandler http.Handler = mux
	if n.config.RPC.IsCorsEnabled() {
	corsMiddleware := cors.New(cors.Options{
	AllowedOrigins: n.config.RPC.CORSAllowedOrigins,
	AllowedMethods: n.config.RPC.CORSAllowedMethods,
	AllowedHeaders: n.config.RPC.CORSAllowedHeaders,
	})
	rootHandler = corsMiddleware.Handler(mux)
	}
	if n.config.RPC.IsTLSEnabled() {
	go rpcserver.StartHTTPAndTLSServer(
	listener,
	rootHandler,
	n.config.RPC.CertFile(),
	n.config.RPC.KeyFile(),
	rpcLogger,
	config,
	)
	} else {
	go rpcserver.StartHTTPServer(
	listener,
	rootHandler,
	rpcLogger,
	config,
	)
	}
	listeners[i] = listener
	}
	if rebuildAddresses {
	n.config.RPC.ListenAddress = joinListenerAddresses(listeners)
	}
	return listeners, nil
	}

gno/tm2/pkg/bft/node/node.go

Lines 754 to 756 in 15d119f

	if err != nil {
	return nil, err
	}

having been added in a slice in

gno/tm2/pkg/bft/node/node.go

Line 785 in 15d119f

listeners[i] = listener

Remedy

We can use named returns to ensure that on any error, all the other listeners are closed.