internal/hcs cleanup; and callbackNumber type

.github/workflows/ci.yml

@@ -36,7 +36,7 @@ jobs:
       - name: Run golangci-lint
         uses: golangci/golangci-lint-action@v9
         with:
-          version: v2.1
+          version: v2.4
           args: >-
             --verbose
             --max-issues-per-linter=0

internal/hcs/callback.go

@@ -3,20 +3,24 @@
 package hcs
 
 import (
+	"context"
 	"fmt"
 	"sync"
+	"sync/atomic"
 	"syscall"
 
+	"github.com/sirupsen/logrus"
+
 	"github.com/Microsoft/hcsshim/internal/interop"
+	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/logfields"
 	"github.com/Microsoft/hcsshim/internal/vmcompute"
-	"github.com/sirupsen/logrus"
 )
 
 var (
-	nextCallback    uintptr
-	callbackMap     = map[uintptr]*notificationWatcherContext{}
+	// used to lock [callbackMap].
 	callbackMapLock = sync.RWMutex{}
+	callbackMap     = map[callbackNumber]*notificationWatcherContext{}
 
 	notificationWatcherCallback = syscall.NewCallback(notificationWatcher)
 
@@ -87,6 +91,31 @@ func (hn hcsNotification) String() string {
 	}
 }
 
+// HCS callbacks take the form:
+//
+//	typedef void (CALLBACK *HCS_NOTIFICATION_CALLBACK)(
+//		_In_ DWORD notificationType,
+//		_In_opt_ void*  context,
+//		_In_ HRESULT notificationStatus,
+//		_In_opt_ PCWSTR notificationData
+//		);
+//
+// where the context is a pointer to the data that is associated with a particular notification.
+//
+// However, since Golang can freely move structs, pointer values are not stable.
+// Therefore, interpret the pointer as the unique ID of a [notificationWatcherContext]
+// stored in [callbackMap].
+//
+// Note: Pointer stability via converting to [unsafe.Pointer] for syscalls is only guaranteed
+// until the syscall returns, and the same pointer value is therefore invalid across different
+// syscall invocations.
+// See point (4) of the [unsafe.Pointer] documentation.
+type callbackNumber uintptr
+
+var callbackCounter atomic.Uintptr
+
+func nextCallback() callbackNumber { return callbackNumber(callbackCounter.Add(1)) }
+
 type notificationChannel chan error
 
 type notificationWatcherContext struct {
@@ -132,30 +161,38 @@ func closeChannels(channels notificationChannels) {
 	}
 }
 
-func notificationWatcher(notificationType hcsNotification, callbackNumber uintptr, notificationStatus uintptr, notificationData *uint16) uintptr {
+func notificationWatcher(
+	notificationType hcsNotification,
+	callbackNum callbackNumber,
+	notificationStatus uintptr,
+	notificationData *uint16,
+) uintptr {
+	entry := log.G(context.Background()).WithFields(logrus.Fields{
+		logfields.CallbackNumber: callbackNum,
+		"notification-type":      notificationType.String(),
+	})
+
 	var result error
 	if int32(notificationStatus) < 0 {
 		result = interop.Win32FromHresult(notificationStatus)
 	}
 
 	callbackMapLock.RLock()
-	context := callbackMap[callbackNumber]
+	callbackCtx := callbackMap[callbackNum]
 	callbackMapLock.RUnlock()
 
-	if context == nil {
+	if callbackCtx == nil {
+		entry.Warn("received HCS notification for unknown callback number")
 		return 0
 	}
 
-	log := logrus.WithFields(logrus.Fields{
-		"notification-type": notificationType.String(),
-		"system-id":         context.systemID,
-	})
-	if context.processID != 0 {
-		log.Data[logfields.ProcessID] = context.processID
+	entry.Data[logfields.SystemID] = callbackCtx.systemID
+	if callbackCtx.processID != 0 {
+		entry.Data[logfields.ProcessID] = callbackCtx.processID
 	}
-	log.Debug("HCS notification")
+	entry.Debug("received HCS notification")
 
-	if channel, ok := context.channels[notificationType]; ok {
+	if channel, ok := callbackCtx.channels[notificationType]; ok {
 		channel <- result
 	}