util_linux: add connection close in setupIO #4780
Conversation
If an error occurs while creating a file descriptor using
socket, err := uc.File(), this error is returned to the calling
runner.run function. The runner.run function also simply returns
this error, and, as a result, the connection created in the line
conn, err := net.Dial("unix", sockpath) remains open.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Tigran Sogomonian <[email protected]>
utils_linux.go
Outdated
| @@ -123,11 +123,13 @@ func setupIO(process *libcontainer.Process, container *libcontainer.Container, c | |||
| } | |||
| uc, ok := conn.(*net.UnixConn) | |||
| if !ok { | |||
| conn.Close() | |||
There was a problem hiding this comment.
Thank you for your contribution.
In practice, omitting these close operations doesn't cause any harm, since runc will exit anyway if an error occurs during the setupIO call.
However, from a code style and best practices perspective, I think we should always close the socket connection once an error is encountered after the connection has been established.
Considering both aspects, LGTM.
utils_linux.go
Outdated
| @@ -123,11 +123,13 @@ func setupIO(process *libcontainer.Process, container *libcontainer.Container, c | |||
| } | |||
| uc, ok := conn.(*net.UnixConn) | |||
| if !ok { | |||
| conn.Close() | |||
There was a problem hiding this comment.
I don't understand, if the result is not ok can the connection still be open. Are you sure?
utils_linux.go
Outdated
| return nil, errors.New("casting to UnixConn failed") | ||
| } | ||
| t.postStart = append(t.postStart, uc) | ||
| socket, err := uc.File() | ||
| if err != nil { | ||
| conn.Close() |
There was a problem hiding this comment.
Let's avoid having to do this on every exit path. Please create a defer after the connection is established, that is an inline function and does a conn.Close only if we are returning an error.
There was a problem hiding this comment.
And if we put defer, won't we close the connection in any case (success and error) when exiting the function? Or do you mean that a simple defer is not needed?
There was a problem hiding this comment.
We can do a defer that checks if we are returning with error. See for example here:
Lines 123 to 127 in c3d127f
You defer a function, and you have a named return. Then, in that function you check if the error we are returning is not nil, then you do the close. I think we can use the same here, can't we?
There was a problem hiding this comment.
I do it like that but have failed in CI. What's wrong?
Make connection close using defer. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Tigran Sogomonian <[email protected]>
utils_linux.go
Outdated
| conn, err := net.Dial("unix", sockpath) | ||
| if err != nil { | ||
| return nil, err | ||
| conn, Err := net.Dial("unix", sockpath) |
There was a problem hiding this comment.
You can keep the original err variable here — there's no need to change it to Err.
Since the function uses a named return value, assigning err will automatically set the return value correctly.
utils_linux.go
Outdated
| socket, err := uc.File() | ||
| if err != nil { | ||
| return nil, err | ||
| socket, Err := uc.File() |
There was a problem hiding this comment.
And the same here, you don't need to modify err to Err.
Fix Err to err Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Tigran Sogomonian <[email protected]>
| defer func() { | ||
| if Err != nil { | ||
| conn.Close() | ||
| } | ||
| }() |
There was a problem hiding this comment.
The uc.File() call below is also missing a close.
Not sure when we can close it, though. Maybe it's handled by the caller, can you check?
If an error occurs while creating a file descriptor using socket, err := uc.File(), this error is returned to the calling runner.run function. The runner.run function also simply returns this error, and, as a result, the connection created in the line conn, err := net.Dial("unix", sockpath) remains open.
Found by Linux Verification Center (linuxtesting.org) with SVACE.