remotedb/grpcdb: fix net.Listener leak in ListenAndServe #269
Conversation
Uses Go's named return value feature to properly check and ensure that if any error is encountered, that we properly invoke .Close for the bound net listener inside of ListenAndServe. Fixes tendermint#268
odeke-em
requested review from
alexanderbez,
cmwaters,
creachadair,
tac0turtle,
tychoish and
williambanfield
as code owners
| ln, err := net.Listen("tcp", addr) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
Is there some reason we shouldn't simply defer the close unconditionally? This function blocks until the server terminates anyway, and we're not checking the result of the Close call anyway.
There was a problem hiding this comment.
Yes:
- It is the simplest and most non-invasive fix, as I've pushed up :-)
- If at all NewServer returns a value whose .Serve method has goroutines that might do other work on the listener, or also later on invokes .Close on the listener, that could cause problems and the biggest one being an already closed error being reported (something that I've seen in many code bases)
There was a problem hiding this comment.
The listener is created in this scope, it cannot be closed anywhere else.
There was a problem hiding this comment.
I don't know that this works.
Additionally this database implementation is not, as far as I know ever used, and in light of that, I would be more comfortable deleting it rather than attempting to fix it, but if there are production users, then we can pursue this more.
| @@ -16,11 +16,17 @@ import ( | |||
| // ListenAndServe is a blocking function that sets up a gRPC based | |||
| // server at the address supplied, with the gRPC options passed in. | |||
| // Normally in usage, invoke it in a goroutine like you would for http.ListenAndServe. | |||
| func ListenAndServe(addr, cert, key string, opts ...grpc.ServerOption) error { | |||
| func ListenAndServe(addr, cert, key string, opts ...grpc.ServerOption) (rerr error) { | |||
There was a problem hiding this comment.
can you explain how rerr is ever mutated so that line 25 could ever be true?
There was a problem hiding this comment.
Go has a feature called "named results" https://go.dev/ref/spec#Function_types whereby if you name a return value, in a defer you can check against that value so what the last return set will now be captured by that return.
There was a problem hiding this comment.
The return at the end sets it. (Normal returns also set named return values)
Codecov Report
@@ Coverage Diff @@
## master #269 +/- ##
==========================================
- Coverage 68.54% 68.41% -0.13%
==========================================
Files 27 27
Lines 2130 2134 +4
==========================================
Hits 1460 1460
- Misses 595 599 +4
Partials 75 75
|
|
@tychoish thanks for the review! In regards to deleting code, I think perhaps I can leave that decision to the maintainers, I am just fixing security issues that I identified. |
It's difficult to tell. A cursory search on cs.github.com doesn't turn up any obvious uses, and if that's properly indicative it suggests maybe the whole |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Uses Go's named return value feature to properly check and
ensure that if any error is encountered, that we properly
invoke .Close for the bound net listener inside of ListenAndServe.
Fixes #268