microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.07k stars 87 forks source link

IStream.Seek has optional parameter plibNewPosition, yet is defined as non-optional output parameter #1040

Closed BasTossings closed 1 year ago

BasTossings commented 1 year ago

Actual behavior

In CSWin32's IStream definition, Seek is defined as follows (in the c# version at least):

void Seek(long dlibMove, global::System.IO.SeekOrigin dwOrigin, out ulong plibNewPosition)

According to the official documentation however, plibNewPosition can be set to NULL if the caller is not interested in the new position.

At the moment, if you implement the IStream interface in managed code, and the caller chooses to set plibNewPosition to NULL, the managed implementation throws a System.NullReferenceException when you set the plibNewPosition, with no way to handle this case correctly as far as I can tell.

Expected behavior

IStream.Seek should probably be defined as follows:

unsafe void Seek(long dlibMove, global::System.IO.SeekOrigin dwOrigin, ulong* plibNewPosition)

Repro steps

  1. NativeMethods.txt content:

    IStream
  2. NativeMethods.json content (if present):

    {
    "$schema": "https://aka.ms/CsWin32.schema.json",
    "public": false,
    "allowMarshaling": true
    }

Context

AArnott commented 1 year ago

Great report.

First, I think this workaround should help you:

public unsafe HRESULT Read(void* pv, uint cb, out uint pcbRead)
{
    fixed (uint* ppcbRead = &pcbRead)
    {
        if (ppcbRead is not null)
        {
            pcbRead = 5;
        }
    }

    return HRESULT.S_OK;
}

This should allow you to detect a null argument passed to you so you can avoid throwing NRE.

The simpler way we could possibly expose to you is to declare the method with uint* pcbRead instead of out uint pcbRead.

The metadata includes the point that this parameter is optional, so exposing this as a pointer in the optional case makes sense.

BasTossings commented 1 year ago

Thanks for the super fast response!