Skip to content

Commit 44d6c7d

Browse files
nybidarigvisor-bot
authored andcommitted
Fix double close of fds in FDWriter.Close
This mitigates a race condition where a reused host file descriptor could be unexpectedly closed by a delayed double close call. PiperOrigin-RevId: 943354204
1 parent abf5927 commit 44d6c7d

3 files changed

Lines changed: 61 additions & 9 deletions

File tree

pkg/sentry/state/stateio/BUILD

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ go_library(
2525
deps = [
2626
"//pkg/abi/linux",
2727
"//pkg/aio",
28+
"//pkg/atomicbitops",
2829
"//pkg/hostarch",
2930
"//pkg/log",
3031
"//pkg/memutil",
@@ -41,6 +42,7 @@ go_test(
4142
srcs = [
4243
"bufreader_test.go",
4344
"bufwriter_test.go",
45+
"fdwriter_test.go",
4446
"ioreader_test.go",
4547
"iowriter_test.go",
4648
],

pkg/sentry/state/stateio/fdwriter.go

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"golang.org/x/sys/unix"
2121
"gvisor.dev/gvisor/pkg/abi/linux"
2222
"gvisor.dev/gvisor/pkg/aio"
23+
"gvisor.dev/gvisor/pkg/atomicbitops"
2324
"gvisor.dev/gvisor/pkg/log"
2425
"gvisor.dev/gvisor/pkg/sentry/hostfd"
2526
"gvisor.dev/gvisor/pkg/sentry/memmap"
@@ -30,7 +31,7 @@ type FDWriter struct {
3031
NoRegisterClientFD
3132

3233
// fd is the file descriptor. fd is immutable.
33-
fd int32
34+
fd atomicbitops.Int32
3435

3536
// maxWriteBytes and maxRanges are AsyncWriter parameters. Both are
3637
// immutable.
@@ -126,7 +127,7 @@ func NewFDWriter(fd int32, maxWriteBytes uint64, maxRanges, maxParallel int) *FD
126127
}
127128

128129
return &FDWriter{
129-
fd: fd,
130+
fd: atomicbitops.FromInt32(fd),
130131
maxWriteBytes: uint32(min(maxWriteBytes, uint64(linux.MAX_RW_COUNT))),
131132
maxRanges: uint32(min(maxRanges, hostfd.MaxReadWriteIov)),
132133
preextend: preextend,
@@ -138,8 +139,12 @@ func NewFDWriter(fd int32, maxWriteBytes uint64, maxRanges, maxParallel int) *FD
138139

139140
// Close implements AsyncWriter.Close.
140141
func (w *FDWriter) Close() error {
142+
fd := w.fd.Swap(-1)
143+
if fd == -1 {
144+
return nil
145+
}
141146
w.q.Destroy()
142-
return unix.Close(int(w.fd))
147+
return unix.Close(int(fd))
143148
}
144149

145150
// MaxWriteBytes implements AsyncWriter.MaxWriteBytes.
@@ -159,7 +164,7 @@ func (w *FDWriter) MaxParallel() int {
159164

160165
// AddWrite implements AsyncWriter.AddWrite.
161166
func (w *FDWriter) AddWrite(id int, _ SourceFile, _ memmap.FileRange, srcMap []byte) {
162-
aio.Write(w.q, uint64(id), w.fd, w.off, srcMap)
167+
aio.Write(w.q, uint64(id), w.fd.Load(), w.off, srcMap)
163168
w.inflight[id] = fdWrite{
164169
off: w.off,
165170
total: uint64(len(srcMap)),
@@ -170,7 +175,7 @@ func (w *FDWriter) AddWrite(id int, _ SourceFile, _ memmap.FileRange, srcMap []b
170175

171176
// AddWritev implements AsyncWriter.AddWritev.
172177
func (w *FDWriter) AddWritev(id int, total uint64, _ SourceFile, _ []memmap.FileRange, srcMaps []unix.Iovec) {
173-
aio.Writev(w.q, uint64(id), w.fd, w.off, srcMaps)
178+
aio.Writev(w.q, uint64(id), w.fd.Load(), w.off, srcMaps)
174179
w.inflight[id] = fdWrite{
175180
off: w.off,
176181
total: total,
@@ -186,10 +191,10 @@ func (w *FDWriter) Wait(cs []Completion, minCompletions int) ([]Completion, erro
186191
if w.preextend && w.fileSize < w.off {
187192
newSize := max(w.off, w.reserved)
188193
w.reserved = 0
189-
if err := unix.Ftruncate(int(w.fd), newSize); err != nil {
194+
if err := unix.Ftruncate(int(w.fd.Load()), newSize); err != nil {
190195
// This can occur if e.g. the file is FUSE-backed, and the FUSE
191196
// server doesn't support file extension.
192-
log.Infof("stateio.FDWriter: ftruncate(%d, %d) failed: %v", w.fd, newSize, err)
197+
log.Infof("stateio.FDWriter: ftruncate(%d, %d) failed: %v", w.fd.Load(), newSize, err)
193198
w.preextend = false
194199
// Update w.fileSize assuming that all writes complete
195200
// successfully, as below.
@@ -240,9 +245,9 @@ retry:
240245
inflight.done = done
241246
inflight.src = inflight.src.DropFirst(n)
242247
if inflight.src.Mapping != nil {
243-
aio.Write(w.q, aioC.ID, w.fd, inflight.off, inflight.src.Mapping)
248+
aio.Write(w.q, aioC.ID, w.fd.Load(), inflight.off, inflight.src.Mapping)
244249
} else {
245-
aio.Writev(w.q, aioC.ID, w.fd, inflight.off, inflight.src.Iovecs)
250+
aio.Writev(w.q, aioC.ID, w.fd.Load(), inflight.off, inflight.src.Iovecs)
246251
}
247252
// w.q may be an aio.LinuxQueue, in which case we need to call
248253
// w.q.Wait() again to submit this write.
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright 2026 The gVisor Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package stateio
16+
17+
import (
18+
"testing"
19+
20+
"golang.org/x/sys/unix"
21+
)
22+
23+
func TestFDWriterDoubleClose(t *testing.T) {
24+
var fds [2]int
25+
if err := unix.Pipe(fds[:]); err != nil {
26+
t.Fatalf("Pipe failed: %v", err)
27+
}
28+
readFD := fds[0]
29+
writeFD := fds[1]
30+
31+
defer unix.Close(readFD)
32+
33+
// NewFDWriter takes ownership of writeFD.
34+
w := NewFDWriter(int32(writeFD), 4096, 1, 1)
35+
36+
// First close should succeed.
37+
if err := w.Close(); err != nil {
38+
t.Fatalf("First Close() failed: %v", err)
39+
}
40+
41+
// Second close should also succeed (idempotent) and not return EBADF.
42+
if err := w.Close(); err != nil {
43+
t.Fatalf("Second Close() failed: %v", err)
44+
}
45+
}

0 commit comments

Comments
 (0)