Fix serial console attachment silently disabled by wrong error check#217
Open
jlagedo wants to merge 2 commits into
Open
Fix serial console attachment silently disabled by wrong error check#217jlagedo wants to merge 2 commits into
jlagedo wants to merge 2 commits into
Conversation
newVZFileHandleSerialPortAttachment checked the error out-parameter pointer (`if (error != nil)`), which is always non-nil since callers pass the address of their error variable. The function therefore returned nil before building the attachment, silently disabling the serial console. Check the duplicated file handle instead, matching newVZFileHandleNetworkDeviceAttachment. Add a regression test asserting the constructor returns a non-NULL attachment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cfergeau
reviewed
May 26, 2026
| @autoreleasepool { | ||
| // Check the returned handle, not `error`: `error` is the void** out-param (always | ||
| // non-nil), so `if (error != nil)` is always true. newFileHandleDupFd returns nil | ||
| // and sets *error only on failure. Matches newVZFileHandleNetworkDeviceAttachment. |
Contributor
There was a problem hiding this comment.
Not sure this comment is required once the bug is fixed, there’s a test case to prevent regressions, and git blame will also give an explanation.
Apart from this, looks good to me, great catch!
Author
There was a problem hiding this comment.
Done, comment removed in d77a0dd — thanks for the review!
Funny story: I wasn't even hunting this bug. I was trying to debug a boot error and turned on the serial console to see what was happening… except nothing ever showed up. Turns out the console was being silently disabled the whole time by this broken error check. So I went looking for my boot bug and accidentally found a different one. 😅
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
newVZFileHandleSerialPortAttachmentchecked the error out-parameter pointer instead of the returned file handle, which silently disabled the file-handle serial console.Because callers always pass the address of their error variable (
&nserrPtrinserial_console.go),error != nilis always true, so the function returnednilbefore ever building the attachment. On the Go side this produced a non-nil*FileHandleSerialPortAttachmentwrapping a NULL Objective-C object with a nil error — a silent failure. The guest's serial console (console=hvc0) had nowhere to write.Fix
Check the duplicated handle returned by
newFileHandleDupFd, which returnsniland sets*erroronly ondup()failure. This is the same pattern already used bynewVZFileHandleNetworkDeviceAttachment(virtualization_11.m) andnewVZDiskBlockDeviceStorageDeviceAttachment(virtualization_14.m).This also aligns with Apple's Cocoa convention: success/failure is signaled by the return value, and the
NSErrorout-parameter is only consulted on failure.Why existing tests didn't catch it
The bootable-VM integration test (
TestRun) talks to the guest over vsock/SSH, not the serial console. A serial port with a nil attachment is valid config, soValidate()andStart()succeed; only the kernel boot log was lost, and nothing asserted on it. Added a focused unit test that asserts the constructor returns a non-NULL attachment.Test evidence
Run via
make test(codesigns each test binary with thecom.apple.security.virtualizationentitlement; barego testaborts with an uncaughtNSInvalidArgumentExceptionwhen creating a VM).New regression test:
Verified the test actually catches the bug — temporarily reverting the checks to
if (error != nil)makes it fail:Real VM boot now shows serial console output (
console=hvc0) and completes the full SSH-over-vsock flow:Full suite:
(
TestRunIssue124SKIP — gated behindTEST_ISSUE_124=1.)Local environment
Test plan
make testpasses (full suite, incl. VM boot tests)go vet ./...clean