Use `syscall.Dup` or such in `NewDiskBlockDeviceStorageDeviceAttachment`?

[cfergeau]

Describe the bug

This came up in crc-org/vfkit#373 (comment)
The code roughly does:

		f, err := os.OpenFile(devPath, open_flags, 0)
		if err != nil {
			return nil, fmt.Errorf("error opening file: %v", err)
		}

		attachment, err := vz.NewDiskBlockDeviceStorageDeviceAttachment(f, conf.ReadOnly, vz.DiskSynchronizationModeFull)
		if err != nil {
			_ = f.Close()
			return nil, fmt.Errorf("error creating disk attachment: %v", err)
		}
		// create device with attachment and add it to VM

After this function returns, f can be garbage collected at any time, which will close its file descriptor.
The relevant code in Code-Hex/vz is https://github.com/Code-Hex/vz/blob/c17cd79c8418115e1fcbf98c78f53bc62fd0c972/storage.go#L384C6-L408 , it uses file.Fd() directly, and then it’s used as

vz/virtualization_14.m

Lines 40 to 53 in c17cd79

	void *newVZDiskBlockDeviceStorageDeviceAttachment(int fileDescriptor, bool readOnly, int syncMode, void **error)
	{
	#ifdef INCLUDE_TARGET_OSX_14
	if (@available(macOS 14, *)) {
	NSFileHandle *fileHandle = [[NSFileHandle alloc] initWithFileDescriptor:fileDescriptor];
	return [[VZDiskBlockDeviceStorageDeviceAttachment alloc]
	initWithFileHandle:fileHandle
	readOnly:(BOOL)readOnly
	synchronizationMode:(VZDiskSynchronizationMode)syncMode
	error:(NSError *_Nullable *_Nullable)error];
	}
	#endif
	RAISE_UNSUPPORTED_MACOS_EXCEPTION();
	}

I don’t think NSFileHandle *fileHandle = [[NSFileHandle alloc] initWithFileDescriptor:fileDescriptor]; will dup() the file descriptor.

The end result is that we’ll be getting errors such as this when f gets closed prematurely:

NSLocalizedFailure = "Invalid virtual machine configuration.";
NSLocalizedFailureReason = "The storage device attachment is invalid."

This is a bit misleading as the doc says The file handle is retained by the disk attachment.

To Reproduce
I don’t have an easy reproducer, but storage_test.go can probably trigger this in a new test case for block devices and artificial calls to runtime.GC().
The vfkit reproducer is described at crc-org/vfkit#373 (comment) but has been fixed in the latest iteration (crc-org/vfkit#373 (comment))

Expected behavior
Either NewDiskBlockDeviceStorageDeviceAttachment() works fine even if the *os.File argument is closed after it completes, or its documentation changed to explicitly say that *os.File must not be closed/garbage collected while the attachment is in use.

[cfergeau]

Could be something like this: (untested)

diff --git a/storage.go b/storage.go
index 5670109..9b11c2e 100644
--- a/storage.go
+++ b/storage.go
@@ -13,6 +13,7 @@ import "C"
 import (
        "os"
        "runtime/cgo"
+       "syscall"
        "time"
        "unsafe"
 
@@ -388,10 +389,14 @@ func NewDiskBlockDeviceStorageDeviceAttachment(file *os.File, readOnly bool, syn
 
        nserrPtr := newNSErrorAsNil()
 
+       fd, err := syscall.Dup(int(file.Fd()))
+       if err != nil {
+               return nil, err
+       }
        attachment := &DiskBlockDeviceStorageDeviceAttachment{
                pointer: objc.NewPointer(
                        C.newVZDiskBlockDeviceStorageDeviceAttachment(
-                               C.int(file.Fd()),
+                               C.int(fd),
                                C.bool(readOnly),
                                C.int(syncMode),
                                &nserrPtr,
@@ -402,6 +407,7 @@ func NewDiskBlockDeviceStorageDeviceAttachment(file *os.File, readOnly bool, syn
                return nil, err
        }
        objc.SetFinalizer(attachment, func(self *DiskBlockDeviceStorageDeviceAttachment) {
+               _ = syscall.Close(fd)
                objc.Release(self)
        })
        return attachment, nil

[cfergeau]

Thinking more about it, the first decision to make is how the NewDiskBlockDeviceStorageDeviceAttachment API is expected to be used:
1.

f, err := os.OpenFile(…)
if err != nil {
    return err
}
defer f.Close()
attachment, err := vz.NewDiskBlockDeviceStorageDeviceAttachment(f, …)
if err != nil {
    return err
}
// the attachment uses a dup'ed file descriptor, it’s independent from f
// f can be closed without interfering with the attachment

f, err := os.OpenFile(…)
if err != nil {
    return err
}
attachment, err := vz.NewDiskBlockDeviceStorageDeviceAttachment(f, …)
if err != nil {
    f.Close()
    return err
}
// f should be closed if there’s an error
// if an attachment was returned, f must not be closed until the attachment is no longer used by the VM
// in particular, this means f must not be garbage collected while attachment is being used as this would close it

f, err := os.OpenFile(…)
if err != nil {
    return err
}
attachment, err := vz.NewDiskBlockDeviceStorageDeviceAttachment(f, …)
if err != nil {
    f.Close()
    return err
}
// f should be closed if there’s an error
// if an attachment was returned, the attachment now owns f and will close it when it’s no longer needed

The current behaviour of the API is 2., which wаs unexpected/not crystal clear from the API doc.
Behaviour 1. integrates better with defer/go error handling, but all 3 options can work as long as the documentation is clear about it.

[cfergeau]

Current behaviour (2) is consistent with

vz/network.go

Lines 180 to 186 in c17cd79

	// NewFileHandleNetworkDeviceAttachment initialize the attachment with a file handle.
	//
	// file parameter is holding a connected datagram socket.
	//
	// This is only supported on macOS 11 and newer, error will
	// be returned on older versions.
	func NewFileHandleNetworkDeviceAttachment(file *os.File) (*FileHandleNetworkDeviceAttachment, error) {

though.