socketpair_unix: avoid double close(), set FD_CLOEXEC#66
socketpair_unix: avoid double close(), set FD_CLOEXEC#66fuweid merged 1 commit intocontainerd:mainfrom
Conversation
f7892a6 to
5633508
Compare
|
ping @klihub (initial author) |
Yeah, we haven't really had the time to impement and test NRI for Windows, yet. That said, on Windows, SocketPair emulates a socketpair using two ends of a pre-connected TCPv4 socket, which are then stored as sys.Handle's. Shouldn't that protect us from a similar double-close error ? |
5633508 to
3ae338f
Compare
c4da6cf to
c1a14af
Compare
|
@klihub just pushed a version fixing windows and moving all the common functions in socketpair.go (windows part not tested) |
|
Hi @champtar would you please file a ticket to track window part and focus on linux part in this pull request? I don't have too much knowledge on windows platform actually. we can ask other maintainers to take over this. Thanks |
+1 I would also leave Windows out of this now and focus on fixing the bug on un*x, since we anyway don't have all the necessary functionality in place for Windows. Sorry if my earlier question/comment was misleading and could be interpreted otherwise. |
c1a14af to
3ae338f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 64.50% 64.58% +0.07%
==========================================
Files 9 10 +1
Lines 1834 1838 +4
==========================================
+ Hits 1183 1187 +4
Misses 500 500
Partials 151 151 ☔ View full report in Codecov by Sentry. |
|
Pushed back the unix only changes, I'll open a second PR once this one is merged |
We were calling the close() syscall multiple time
with the same fd number, leading to random issues like
closing containerd stream port.
In newLaunchedPlugin() we have:
sockets, _ := net.NewSocketPair()
defer sockets.Close()
conn, _ := sockets.LocalConn()
peerFile := sockets.PeerFile()
defer func() {
peerFile.Close()
if retErr != nil {
conn.Close()
}
}()
cmd.Start()
so we were doing:
close(local) (in LocalConn())
cmd.Start()
close(peer) (peerFile.Close())
close(local) (sockets.Close())
close(peer) (sockets.Close())
If the NRI plugin that we launch with cmd.Start() is not cached or
the system is a bit busy, cmd.Start() gives a large enough window
for another goroutine to open a file that will get the same fd number
as local was, thus being closed by accident.
Fix the situation by storing os.Files instead of ints, so that closing
multiple times just returns an error (that we ignore).
Also set FD_CLOEXEC on the sockets so we don't leak them.
Fixes 1da2cdf
Signed-off-by: Etienne Champetier <e.champetier@ateme.com>
3ae338f to
5d0b52b
Compare
|
@klihub hopefully final version |
We were calling the close() syscall multiple time
with the same fd number, leading to random issues like
closing containerd stream port.
In newLaunchedPlugin() we have:
so we were doing:
If the NRI plugin that we launch with cmd.Start() is not cached or
the system is a bit busy, cmd.Start() gives a large enough window
for another goroutine to open a file that will get the same fd number
as local was, thus being closed by accident.
Fix the situation by storing os.Files instead of ints, so that closing
multiple times just returns an error.
Also set FD_CLOEXEC on the sockets so we don't leak them.
Fixes 1da2cdf