gatherCandidatesRelay can leak sockets

[lrezek]

Your environment.

• Version: 0.5.13

What did you do?

When gathering relay candidates with invalid credentials, a socket remains open after the peer is closed. This can be checked with lsof on a linux system.

What did you expect?

That all open sockets would be closed when the peer is closed.

What happened?

A socket remained open.

Debugging Notes

I've narrowed down this issue to gatherCandidatesRelay in gather.go. First we create a network socket, using:

locConn, err := a.net.ListenPacket(network, "0.0.0.0:0")
if err != nil {
    return err
}

After that, there are a variety of places we can return from the function with an error. In my case, it was the following (because I had some invalid credentials 😄):

relayConn, err := client.Allocate()
if err != nil {
    return err
}

If we encounter an error in any of these cases, the socket created earlier would never be closed (until the process is terminated). Ideally, this function would close the socket if an error occurs, and perhaps log an error.

This may also be an issue with the other candidate types, though I have not run into that yet.

[Sean-Der]

Oh fantastic catch!

This looks like an issue with other candidates as well :( probably much harder to reproduce with other types though.

Would you be up for creating a PR? I love having people involved, the more eyes we get on the code the better :) I know this code much better, so I can help land this quickly!

I would just do something like the following. Feel free to aggressively rewrite code to make it cleaner! As long as we have good test coverage it is a 👍 from me

shouldCloseConn := true
defer func() {
  if shouldCloseConn {
    locConn.Close()
  }
}()
...
...
...
// Last line of the function
shouldCloseConn = false