Skip to content

Protect QEMU vsock descriptors from repeated close - #484

Merged
yummybomb merged 4 commits into
mainfrom
hypeship/fix-vsock-fd-ownership
Sep 18, 2026
Merged

yummybomb merged 4 commits into
mainfrom
hypeship/fix-vsock-fd-ownership

Conversation

@yummybomb

@yummybomb yummybomb commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Why

A failed gRPC handshake can close the same QEMU vsock connection more than once. Our wrapper closed the raw file descriptor on every call. If Linux had already reused that descriptor, the second close could close an unrelated file, including a directory being scanned during GPU cleanup.

What changed

Use os.File to manage the descriptor instead of implementing reads, writes, closes, and deadlines ourselves. The socket stays nonblocking so Go's runtime poller can handle waiting and timeouts. vsockConn only adds the vsock addresses.

This protects against repeated closes and I/O racing with close. A close-once guard alone would not protect the I/O calls.

It also fixes three existing behaviors:

  • A peer disconnect now returns io.EOF, rather than an empty successful read.
  • Closing the connection unblocks pending reads.
  • An expired deadline times out immediately instead of disabling the timeout.

Testing

  • go test -race ./lib/hypervisor/qemu ./lib/guest passed.
  • The new vsock tests passed 50 repetitions under the race detector. They cover descriptor reuse, repeated closes, read/write behavior, EOF, and deadlines.
  • The descriptor-reuse regression fails against the old implementation with bad file descriptor.

Tests use local socket pairs. Not tested against a live AF_VSOCK guest connection.

Independent of #485. No dependency or generated-file changes.

Automated review summary

[!NOTE]
Medium Risk
Changes how guest-facing vsock I/O and teardown behave on Linux, but the shift to stdlib file handling is a targeted safety fix with new regression tests.

Overview
Fixes a bug where repeated closes on the QEMU vsock net.Conn could close an unrelated reused file descriptor (e.g. during GPU cleanup), by handing descriptor lifetime to an embedded *os.File instead of raw unix.Close on every Close().

The dial path no longer switches the socket back to blocking mode; newVsockConn keeps the fd non-blocking and wraps it with os.NewFile so the runtime poller owns read/write, close, and deadlines. Custom Read/Write/Close/SetDeadline implementations are removed; vsockConn only supplies vsock LocalAddr/RemoteAddr.

Adds Linux unit tests (socket pairs) for descriptor reuse after close, concurrent repeated closes, read/write/EOF, close unblocking reads, and deadline behavior.

Reviewed by Cursor Bugbot for commit 071e8b4. Bugbot is set up for automated code reviews on this repo. Configure here.

@yummybomb
yummybomb marked this pull request as ready for review September 17, 2026 17:28
@yummybomb
yummybomb requested review from hiroTamada and removed request for sjmiller609 September 17, 2026 17:57

@hiroTamada hiroTamada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed — descriptor ownership and deadline changes look sound. no blocking findings.

non-blocking test suggestion:

  • lib/hypervisor/qemu/vsock_test.go:80–87 — consider ensuring the read is actually waiting before closing; currently this can pass by exercising only read-after-close.

validated locally with go test -race ./lib/hypervisor/qemu ./lib/guest and 50 repetitions of the vsock tests under the race detector. live AF_VSOCK guest behavior was not tested.

@yummybomb
yummybomb merged commit db82931 into main Sep 18, 2026
9 checks passed
@yummybomb
yummybomb deleted the hypeship/fix-vsock-fd-ownership branch September 18, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants