Bug Report: Race Condition in camera_windows.go Causes Panic on Close

[vickyvnvn]

Bug Report: Race Condition in camera_windows.go Causes Panic on Close

Summary

There is a race condition in pkg/driver/camera/camera_windows.go that causes a panic ("send on closed channel") when closing the camera on Windows. The DirectShow callback (imageCallback) can attempt to send to a closed channel because the lock is released before the send operation.

Environment

• OS: Windows 10/11
• Go: 1.21+
• pion/mediadevices: v0.8.0

Steps to Reproduce

1. Open a camera using mediadevices.GetUserMedia()
2. Start reading frames from the video track
3. Close the track/stream after a few seconds
4. Panic occurs: panic: send on closed channel

Stack Trace

panic: send on closed channel

goroutine 17 [running, locked to thread]:
github.com/pion/mediadevices/pkg/driver/camera.imageCallback(0x7ff63aefa201?)
    .../pkg/driver/camera/camera_windows.go:85 +0xcb

Root Cause Analysis

The bug is in the imageCallback function and Close method interaction:

Current Code (Buggy)

//export imageCallback
func imageCallback(cam uintptr) {
    callbacksMu.RLock()
    cb, ok := callbacks[uintptr(unsafe.Pointer(cam))]  // BUG #1: Double conversion
    callbacksMu.RUnlock()  // Lock released here
    if !ok {
        return
    }

    copy(cb.bufGo, cb.buf)
    cb.ch <- cb.bufGo  // PANIC: Channel may be closed!
}

func (c *camera) Close() error {
    callbacksMu.Lock()
    key := uintptr(unsafe.Pointer(c.cam))
    if _, ok := callbacks[key]; ok {
        delete(callbacks, key)
    }
    callbacksMu.Unlock()
    close(c.ch)  // Channel closed here
    // ...
}

Race Condition Timeline

Thread A (imageCallback)              Thread B (Close)
─────────────────────────────────────────────────────────
1. RLock()
2. Get cb from map ✓
3. RUnlock()
                                      4. Lock()
                                      5. Delete from map
                                      6. Unlock()
                                      7. close(c.ch) ← Channel closed!
8. cb.ch <- data ← PANIC!

Bug #1: Incorrect Key Conversion

// WRONG: cam is already uintptr, this creates wrong key
cb, ok := callbacks[uintptr(unsafe.Pointer(cam))]

// CORRECT: Use cam directly
cb, ok := callbacks[cam]

When cam (which is already uintptr) is cast to unsafe.Pointer and back, it may produce a different value, causing the callback to never find the registered camera in the map.

Bug #2: Race Between Lock Release and Channel Send

The RLock is released before the channel send, allowing Close() to close the channel in between.

Proposed Fix

Option 1: Add closed Flag + Buffered Channel (Recommended)

type camera struct {
    name   string
    cam    *C.camera
    ch     chan []byte
    buf    []byte
    bufGo  []byte
    closed bool  // NEW: Flag to prevent send on closed channel
}

func (c *camera) Open() error {
    c.ch = make(chan []byte, 3)  // Buffered to prevent blocking
    c.closed = false
    // ...
}

//export imageCallback
func imageCallback(cam uintptr) {
    callbacksMu.RLock()
    cb, ok := callbacks[cam]  // FIX: Use cam directly
    
    if !ok || cb == nil || cb.closed {
        callbacksMu.RUnlock()
        return
    }
    
    ch := cb.ch
    buf := cb.buf
    bufGo := cb.bufGo
    callbacksMu.RUnlock()

    // Recover from panic if channel is closed (extra safety)
    defer func() { recover() }()
    
    copy(bufGo, buf)
    ch <- bufGo
}

func (c *camera) Close() error {
    // Mark as closed FIRST while holding lock
    callbacksMu.Lock()
    c.closed = true
    key := uintptr(unsafe.Pointer(c.cam))
    delete(callbacks, key)
    callbacksMu.Unlock()
    
    // Now safe to close channel
    if c.ch != nil {
        close(c.ch)
    }
    
    // ... rest of cleanup
}

Option 2: Hold Lock During Send (Not Recommended)

This would block Close() while sending, which could cause deadlocks if the receiver is not ready.

Diff

diff --git a/pkg/driver/camera/camera_windows.go b/pkg/driver/camera/camera_windows.go
index abc1234..def5678 100644
--- a/pkg/driver/camera/camera_windows.go
+++ b/pkg/driver/camera/camera_windows.go
@@ -25,10 +25,11 @@ var (
 )
 
 type camera struct {
-	name  string
-	cam   *C.camera
-	ch    chan []byte
-	buf   []byte
-	bufGo []byte
+	name   string
+	cam    *C.camera
+	ch     chan []byte
+	buf    []byte
+	bufGo  []byte
+	closed bool
 }
 
 func (c *camera) Open() error {
-	c.ch = make(chan []byte)
+	c.ch = make(chan []byte, 3)
+	c.closed = false
 	c.cam = &C.camera{
 		name: C.CString(c.name),
 	}
@@ -70,15 +71,22 @@ func (c *camera) Open() error {
 
 //export imageCallback
 func imageCallback(cam uintptr) {
 	callbacksMu.RLock()
-	cb, ok := callbacks[uintptr(unsafe.Pointer(cam))]
-	callbacksMu.RUnlock()
-	if !ok {
+	cb, ok := callbacks[cam]
+	
+	if !ok || cb == nil || cb.closed {
+		callbacksMu.RUnlock()
 		return
 	}
+	
+	ch := cb.ch
+	buf := cb.buf
+	bufGo := cb.bufGo
+	callbacksMu.RUnlock()
 
-	copy(cb.bufGo, cb.buf)
-	cb.ch <- cb.bufGo
+	defer func() { recover() }()
+	
+	copy(bufGo, buf)
+	ch <- bufGo
 }
 
 func (c *camera) Close() error {
 	callbacksMu.Lock()
+	c.closed = true
 	key := uintptr(unsafe.Pointer(c.cam))
 	if _, ok := callbacks[key]; ok {
 		delete(callbacks, key)
 	}
 	callbacksMu.Unlock()
-	close(c.ch)
+	
+	if c.ch != nil {
+		close(c.ch)
+	}
 
 	if c.cam != nil {

Additional Notes

1. Buffered Channel: Using make(chan []byte, 3) instead of unbuffered prevents blocking when the reader is temporarily slow.

2. defer recover(): As an extra safety net, recovers from any panic if the channel gets closed between the check and the send.

3. Key Conversion: The original code uses uintptr(unsafe.Pointer(cam)) but cam parameter from CGO is already uintptr. This double conversion may cause key mismatch on some systems.

Test Case

func TestCameraCloseRace(t *testing.T) {
    stream, _ := mediadevices.GetUserMedia(mediadevices.MediaStreamConstraints{
        Video: func(c *mediadevices.MediaTrackConstraints) {
            c.Width = prop.Int(640)
            c.Height = prop.Int(480)
        },
    })
    
    track := stream.GetVideoTracks()[0].(*mediadevices.VideoTrack)
    reader := track.NewReader(false)
    
    // Read a few frames
    for i := 0; i < 10; i++ {
        _, release, _ := reader.Read()
        release()
    }
    
    // Close while DirectShow is still sending frames
    // This should NOT panic
    track.Close()
}

Impact

This bug affects all Windows users of pion/mediadevices who close the camera while it's actively streaming. It's a critical bug that causes application crashes.

References

• CGO callback documentation: https://pkg.go.dev/cmd/cgo
• Go race detector: go test -race