mi: fix native return-value handling (#2537)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

internal/headers/hcs/hcs_test.go

@@ -18,16 +18,38 @@
 package hcs_test
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/prometheus-community/windows_exporter/internal/headers/hcs"
 	"github.com/stretchr/testify/require"
 )
 
+// skipWithoutHCS skips the test if the Host Compute Service is not available,
+// e.g. on hosts without the Containers feature.
+func skipWithoutHCS(t *testing.T, err error) {
+	t.Helper()
+
+	if errors.Is(err, hcs.ErrServiceNotAvailable) {
+		t.Skip("Host Compute Service is not available")
+	}
+}
+
+func TestCreateOperation(t *testing.T) {
+	t.Parallel()
+
+	operation, err := hcs.CreateOperation()
+	require.NoError(t, err)
+	require.NotZero(t, operation)
+
+	hcs.CloseOperation(operation)
+}
+
 func TestGetContainers(t *testing.T) {
 	t.Parallel()
 
 	containers, err := hcs.GetContainers()
+	skipWithoutHCS(t, err)
 	require.NoError(t, err)
 	require.NotNil(t, containers)
 }
@@ -36,6 +58,7 @@ func TestOpenContainer(t *testing.T) {
 	t.Parallel()
 
 	containers, err := hcs.GetContainers()
+	skipWithoutHCS(t, err)
 	require.NoError(t, err)
 
 	if len(containers) == 0 {
@@ -51,5 +74,6 @@ func TestOpenContainerNotFound(t *testing.T) {
 	t.Parallel()
 
 	_, err := hcs.GetContainerStatistics("f3056b79b36ddfe203376473e2aeb4922a8ca7c5d8100764e5829eb5552fe09b")
+	skipWithoutHCS(t, err)
 	require.ErrorIs(t, err, hcs.ErrIDNotFound)
 }

internal/headers/hcs/syscall.go

@@ -18,6 +18,7 @@
 package hcs
 
 import (
+	"errors"
 	"fmt"
 	"unsafe"
 
@@ -38,10 +39,18 @@ var (
 )
 
 // CreateOperation creates a new operation.
+//
+// HcsCreateOperation returns the operation handle, or NULL on failure.
+//
+// https://learn.microsoft.com/en-us/virtualization/api/hcs/reference/hcscreateoperation
 func CreateOperation() (Operation, error) {
-	r1, r2, _ := procHcsCreateOperation.Call(0, 0)
-	if r2 != 0 {
-		return 0, fmt.Errorf("HcsCreateOperation failed: HRESULT 0x%X: %w", r2, Win32FromHResult(r2))
+	r1, _, err := procHcsCreateOperation.Call(0, 0)
+	if r1 == 0 {
+		if errors.Is(err, windows.ERROR_SUCCESS) {
+			err = ErrInvalidHandle
+		}
+
+		return 0, fmt.Errorf("HcsCreateOperation failed: %w", err)
 	}
 
 	return Operation(r1), nil
@@ -102,13 +111,13 @@ func OpenComputeSystem(id string) (ComputeSystem, error) {
 }
 
 func GetComputeSystemProperties(system ComputeSystem, operation Operation, propertyQuery *uint16) error {
-	r1, _, err := procHcsGetComputeSystemProperties.Call(
+	r1, _, _ := procHcsGetComputeSystemProperties.Call(
 		uintptr(system),
 		uintptr(operation),
 		uintptr(unsafe.Pointer(propertyQuery)),
 	)
 	if r1 != 0 {
-		return fmt.Errorf("HcsGetComputeSystemProperties failed: HRESULT 0x%X: %w", r1, err)
+		return fmt.Errorf("HcsGetComputeSystemProperties failed: HRESULT 0x%X: %w", r1, Win32FromHResult(r1))
 	}
 
 	return nil

internal/headers/hcs/types.go

@@ -26,7 +26,11 @@ import (
 
 var (
 	ErrEmptyResultDocument = errors.New("empty result document")
+	ErrInvalidHandle       = errors.New("invalid handle")
 	ErrIDNotFound          = windows.Errno(2151088398)
+	// ErrServiceNotAvailable is HCS_E_SERVICE_NOT_AVAILABLE, returned when the
+	// Host Compute Service is not installed, e.g. without the Containers feature.
+	ErrServiceNotAvailable = windows.Errno(2151088411)
 )
 
 type (

internal/mi/application.go

@@ -105,17 +105,20 @@ type DestinationOptionsFT struct {
 //
 // https://learn.microsoft.com/en-us/windows/win32/api/mi/nf-mi-mi_application_initializev1
 func ApplicationInitialize() (*Application, error) {
+	if err := procMIApplicationInitialize.Find(); err != nil {
+		return nil, err
+	}
+
 	application := &Application{}
 
-	r0, _, err := procMIApplicationInitialize.Call(
+	// The status is the MI_Result return value. The thread's last error is
+	// unrelated and may be set even on success.
+	r0, _, _ := procMIApplicationInitialize.Call(
 		0,
 		uintptr(unsafe.Pointer(applicationID)),
 		0,
 		uintptr(unsafe.Pointer(application)),
 	)
-	if !errors.Is(err, windows.NOERROR) {
-		return nil, fmt.Errorf("syscall returned: %w", err)
-	}
 
 	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
 		return nil, result
@@ -168,10 +171,15 @@ func (application *Application) NewSession(options *DestinationOptions) (*Sessio
 
 	defaultOperationOptions, err := application.NewOperationOptions()
 	if err != nil {
+		_ = session.Close()
+
 		return nil, fmt.Errorf("failed to create default operation options: %w", err)
 	}
 
 	if err = defaultOperationOptions.SetTimeout(5 * time.Second); err != nil {
+		_ = defaultOperationOptions.Delete()
+		_ = session.Close()
+
 		return nil, fmt.Errorf("failed to set timeout: %w", err)
 	}
 
@@ -289,15 +297,19 @@ func (do *DestinationOptions) SetLocale(locale string) error {
 	return nil
 }
 
+// Delete deletes the destination options. MI_DestinationOptions_Delete
+// returns void, so the only error is ErrNotInitialized.
+//
+// https://learn.microsoft.com/en-us/windows/win32/api/mi/nf-mi-mi_destinationoptions_delete
 func (do *DestinationOptions) Delete() error {
-	r0, _, _ := syscall.SyscallN(
+	if do == nil || do.ft == nil {
+		return ErrNotInitialized
+	}
+
+	_, _, _ = syscall.SyscallN(
 		do.ft.Delete,
 		uintptr(unsafe.Pointer(do)),
 	)
 
-	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
-		return result
-	}
-
 	return nil
 }

internal/mi/mi_test.go

@@ -1009,3 +1009,73 @@ func Test_MI_Operation_CloseEarly(t *testing.T) {
 	require.NoError(t, session.Query(&processes, mi.NamespaceRootCIMv2, query, 5*time.Second))
 	require.Equal(t, []win32Process{{Name: "System Idle Process"}}, processes)
 }
+
+// Test_MI_Query_StringArray reads a STRINGA element. Win32_OperatingSystem.MUILanguages
+// is a string[] with at least the installed UI language.
+func Test_MI_Query_StringArray(t *testing.T) {
+	application, err := mi.ApplicationInitialize()
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, application.Close()) })
+
+	session, err := application.NewSession(nil)
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, session.Close()) })
+
+	operation, err := session.QueryInstances(mi.OperationFlagsStandardRTTI, nil, mi.NamespaceRootCIMv2, mi.QueryDialectWQL,
+		"SELECT MUILanguages FROM Win32_OperatingSystem")
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, operation.Close()) })
+
+	instance, _, err := operation.GetInstance()
+	require.NoError(t, err)
+	require.NotNil(t, instance)
+
+	element, err := instance.GetElement("MUILanguages")
+	require.NoError(t, err)
+
+	value, err := element.GetValue()
+	require.NoError(t, err)
+
+	languages, ok := value.([]string)
+	require.True(t, ok, "expected []string, got %T", value)
+	require.NotEmpty(t, languages)
+
+	for _, language := range languages {
+		require.Contains(t, language, "-", "unexpected language tag %q", language)
+	}
+}
+
+// Test_MI_QueryUnmarshal_InvalidClass checks that a failing query is reported
+// through the operation result, not the return value of the void
+// MI_Session_QueryInstances, and that the session stays usable afterwards.
+func Test_MI_QueryUnmarshal_InvalidClass(t *testing.T) {
+	application, err := mi.ApplicationInitialize()
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, application.Close()) })
+
+	session, err := application.NewSession(nil)
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, session.Close()) })
+
+	invalidQuery, err := mi.NewQuery("SELECT Name FROM Win32_DoesNotExist")
+	require.NoError(t, err)
+
+	validQuery, err := mi.NewQuery("SELECT Name FROM Win32_Process WHERE Handle = 0")
+	require.NoError(t, err)
+
+	for range 3 {
+		var processes []win32Process
+
+		err = session.Query(&processes, mi.NamespaceRootCIMv2, invalidQuery, 5*time.Second)
+		require.ErrorIs(t, err, mi.MI_RESULT_INVALID_CLASS)
+		require.Empty(t, processes)
+
+		require.NoError(t, session.Query(&processes, mi.NamespaceRootCIMv2, validQuery, 5*time.Second))
+		require.Equal(t, []win32Process{{Name: "System Idle Process"}}, processes)
+	}
+}

internal/mi/operation.go

@@ -331,30 +331,16 @@ func (o *OperationOptions) SetTimeout(timeout time.Duration) error {
 	return nil
 }
 
-func (o *OperationOptions) Close() error {
-	if o == nil || o.ft == nil {
-		return ErrNotInitialized
-	}
-
-	r0, _, _ := syscall.SyscallN(o.ft.Clone, uintptr(unsafe.Pointer(o)))
-
-	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
-		return result
-	}
-
-	return nil
-}
-
+// Delete deletes the operation options. MI_OperationOptions_Delete returns
+// void, so the only error is ErrNotInitialized.
+//
+// https://learn.microsoft.com/en-us/windows/win32/api/mi/nf-mi-mi_operationoptions_delete
 func (o *OperationOptions) Delete() error {
 	if o == nil || o.ft == nil {
 		return ErrNotInitialized
 	}
 
-	r0, _, _ := syscall.SyscallN(o.ft.Delete, uintptr(unsafe.Pointer(o)))
-
-	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
-		return result
-	}
+	_, _, _ = syscall.SyscallN(o.ft.Delete, uintptr(unsafe.Pointer(o)))
 
 	return nil
 }

internal/mi/session.go

@@ -98,8 +98,11 @@ func (s *Session) TestConnection() error {
 
 	operation := &Operation{}
 
+	// MI_Session_TestConnection returns void. The outcome, e.g. a timeout, is
+	// reported by GetInstance, and the operation must be closed either way.
+	//
 	// ref: https://github.com/KurtDeGreeff/omi/blob/9caa55032a1070a665e14fd282a091f6247d13c3/Unix/scriptext/py/PMI_Session.c#L92-L105
-	r0, _, _ := syscall.SyscallN(
+	_, _, _ = syscall.SyscallN(
 		s.ft.TestConnection,
 		uintptr(unsafe.Pointer(s)),
 		0,
@@ -107,18 +110,16 @@ func (s *Session) TestConnection() error {
 		uintptr(unsafe.Pointer(operation)),
 	)
 
-	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
-		return result
+	_, _, err := operation.GetInstance()
+
+	if closeErr := operation.Close(); err == nil && closeErr != nil {
+		return fmt.Errorf("failed to close operation: %w", closeErr)
 	}
 
-	if _, _, err := operation.GetInstance(); err != nil {
+	if err != nil {
 		return fmt.Errorf("failed to get instance: %w", err)
 	}
 
-	if err := operation.Close(); err != nil {
-		return fmt.Errorf("failed to close operation: %w", err)
-	}
-
 	return nil
 }
 
@@ -160,21 +161,25 @@ func (s *Session) QueryInstances(flags OperationFlags, operationOptions *Operati
 		return nil, err
 	}
 
-	return s.queryInstances(flags, operationOptions, namespaceName, queryDialect, queryExpressionUTF16)
+	return s.queryInstances(flags, operationOptions, namespaceName, queryDialect, queryExpressionUTF16), nil
 }
 
 // queryInstances starts an MI_Session_QueryInstances operation. Nil operationOptions
 // fall back to the session defaults. The caller must close the returned operation.
+//
+// MI_Session_QueryInstances returns void: a failed query still yields an
+// operation, and its status, e.g. a timeout or an invalid class, is reported
+// by [Operation.GetInstance].
 func (s *Session) queryInstances(flags OperationFlags, operationOptions *OperationOptions, namespaceName Namespace,
 	queryDialect QueryDialect, queryExpression Query,
-) (*Operation, error) {
+) *Operation {
 	operation := &Operation{}
 
 	if operationOptions == nil {
 		operationOptions = s.defaultOperationOptions
 	}
 
-	r0, _, _ := syscall.SyscallN(
+	_, _, _ = syscall.SyscallN(
 		s.ft.QueryInstances,
 		uintptr(unsafe.Pointer(s)),
 		uintptr(flags),
@@ -186,11 +191,7 @@ func (s *Session) queryInstances(flags OperationFlags, operationOptions *Operati
 		uintptr(unsafe.Pointer(operation)),
 	)
 
-	if result := ResultError(r0); !errors.Is(result, MI_RESULT_OK) {
-		return nil, result
-	}
-
-	return operation, nil
+	return operation
 }
 
 // unmarshalInstance populates structValue from instance using the `mi` struct tags.
@@ -368,10 +369,7 @@ func (s *Session) QueryUnmarshal[T any](dst *[]T,
 		return err
 	}
 
-	operation, err := s.queryInstances(flags, operationOptions, namespaceName, queryDialect, queryExpression)
-	if err != nil {
-		return fmt.Errorf("failed to query instances: %w", err)
-	}
+	operation := s.queryInstances(flags, operationOptions, namespaceName, queryDialect, queryExpression)
 
 	defer func() {
 		_ = operation.Close()
@@ -420,10 +418,7 @@ func (s *Session) QueryFunc(namespaceName Namespace, queryExpression Query, quer
 		}()
 	}
 
-	operation, err := s.queryInstances(OperationFlagsStandardRTTI, operationOptions, namespaceName, QueryDialectWQL, queryExpression)
-	if err != nil {
-		return fmt.Errorf("failed to query instances: %w", err)
-	}
+	operation := s.queryInstances(OperationFlagsStandardRTTI, operationOptions, namespaceName, QueryDialectWQL, queryExpression)
 
 	// Close cancels the operation if fn aborted it early.
 	defer func() {

internal/mi/value.go

@@ -265,19 +265,7 @@ func (e *Element) GetValue() (any, error) {
 		// Convert the UTF-16 string to a Go string
 		return windows.UTF16PtrToString((*uint16)(unsafe.Pointer(e.value))), nil
 	case ValueTypeSTRINGA:
-		if e.value == 0 {
-			return nil, errors.New("invalid pointer: value is nil")
-		}
-
-		// Assuming array of pointers to UTF-16 strings
-		ptrArray := *(*[]*uint16)(unsafe.Pointer(e.value))
-		strArray := make([]string, len(ptrArray))
-
-		for i, ptr := range ptrArray {
-			strArray[i] = windows.UTF16PtrToString(ptr)
-		}
-
-		return strArray, nil
+		return e.getStringArray(), nil
 	case ValueTypeUINT16A:
 		return e.getUint16Array(), nil
 	default:
@@ -299,3 +287,21 @@ func (e *Element) getUint16Array() []uint16 {
 
 	return out
 }
+
+// getStringArray reads a STRINGA element into a Go []string. The element's
+// value holds the pointer to the array of MI_Char pointers and arrayLen its
+// length. NULL entries become empty strings.
+func (e *Element) getStringArray() []string {
+	if e.value == 0 || e.arrayLen == 0 {
+		return nil
+	}
+
+	src := unsafe.Slice((**uint16)(unsafe.Pointer(e.value)), e.arrayLen)
+
+	out := make([]string, e.arrayLen)
+	for i, ptr := range src {
+		out[i] = windows.UTF16PtrToString(ptr)
+	}
+
+	return out
+}

internal/mi/value_internal_test.go

@@ -24,6 +24,7 @@ import (
 	"unsafe"
 
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
 )
 
 // newElement builds an Element the way Instance.GetElement does, from a raw
@@ -174,3 +175,49 @@ func TestElementIsNull(t *testing.T) {
 	require.True(t, newElement(ValueTypeUINT32, [5]uint64{}, flagNull).IsNull())
 	require.False(t, newElement(ValueTypeUINT32, [5]uint64{}, 0).IsNull())
 }
+
+// nativeAlloc returns zeroed memory outside the Go heap, like the buffers MI
+// returns. The checkptr instrumentation enabled by -race rejects Go heap
+// pointers that are passed through a uintptr, as the MI_Value buffer does.
+func nativeAlloc(t *testing.T, size int) uintptr {
+	t.Helper()
+
+	p, err := windows.LocalAlloc(windows.LPTR, uint32(size))
+	require.NoError(t, err)
+
+	t.Cleanup(func() { _, _ = windows.LocalFree(windows.Handle(p)) })
+
+	return p
+}
+
+// nativeUTF16 copies s as a NUL-terminated UTF-16 string into native memory.
+func nativeUTF16(t *testing.T, s string) uintptr {
+	t.Helper()
+
+	u := windows.StringToUTF16(s)
+	p := nativeAlloc(t, len(u)*2)
+	copy(unsafe.Slice((*uint16)(unsafe.Pointer(p)), len(u)), u)
+
+	return p
+}
+
+func TestElementGetValueStringArray(t *testing.T) {
+	t.Parallel()
+
+	strs := []uintptr{nativeUTF16(t, "en-US"), 0, nativeUTF16(t, "de-DE")}
+
+	array := nativeAlloc(t, len(strs)*int(unsafe.Sizeof(uintptr(0))))
+	copy(unsafe.Slice((*uintptr)(unsafe.Pointer(array)), len(strs)), strs)
+
+	// An MI_StringA is the pointer to the MI_Char* array in word 0 and the
+	// element count in word 1.
+	raw := [5]uint64{uint64(array), uint64(len(strs))}
+
+	got, err := newElement(ValueTypeSTRINGA, raw, 0).GetValue()
+	require.NoError(t, err)
+	require.Equal(t, []string{"en-US", "", "de-DE"}, got)
+
+	got, err = newElement(ValueTypeSTRINGA, [5]uint64{}, flagNull).GetValue()
+	require.NoError(t, err)
+	require.Nil(t, got)
+}