Read serial number (and other strings) may cause memory corruption

[AdrianEggenberger]

I had that my application crashed during transfers with the exception mentioned in the issue #84. The strange thing was that the crash did not always happen at the same time.

I did a lot of changes to my code that calls the hidapi library to eliminate various possible issues. Also I modified my code that read and write calls are only made from the same thread.

After a while I came to the conclusion, that the error occurs when the runtime dicovers issue with memory corruption. This happens independently to the currently running code.

Problem

After digging into the issue by using the code directly from the repository I've found out that the issue was caused by reading the manufacturer, serial number and product identifier when I opened the device. If I removed the reading of these three strings the applicaton worked stable. So I had a close look at these read methods.

The definiton in the hidapi library for these string reading functions look like this:
int HID_API_EXPORT_CALL hid_get_manufacturer_string(hid_device *dev, wchar_t *string, size_t maxlen);
The maxlen parameter description is: The length of the buffer in multiples of wchar_t.

In the library currently a buffer gets allocated by the WCharTString class. The buffer is actually correctly doubled the size of the provided maxmal string length to be able to contain wide chars. The problem is that the value passed to the maxlen parameter is the length of the buffer and not the number of wchars the buffer was created for. This causes that the library asumes that the buffer is larger than it is and probably writes to memory behind the allocated space.

Solution

The solution is to add a parameter "maxLength" to the methods like "GetManufacturerString" in the NativeMethods.cs file. This parameter can than be passed to the invoked call instead of the invalid "buffer.Length" (see below). In the Device.cs the number of allowed characters "maxLength" need then to be passed to the changed methods.

Old:

`
public static int GetManufacturerString(DeviceSafeHandle device, ReadOnlySpan buffer)
{
return GetManufacturerString(device, ref MemoryMarshal.GetReference(buffer), (nuint) buffer.Length);
}

public static int GetProductString(DeviceSafeHandle device, ReadOnlySpan<byte> buffer)
{
    return GetProductString(device, ref MemoryMarshal.GetReference(buffer), (nuint) buffer.Length);
}

public static int GetSerialNumberString(DeviceSafeHandle device, ReadOnlySpan<byte> buffer)
{
    return GetSerialNumberString(device, ref MemoryMarshal.GetReference(buffer), (nuint) buffer.Length);
}

public static int GetIndexedString(DeviceSafeHandle device, int index, ReadOnlySpan<byte> buffer)
{
    return GetIndexedString(device, index, ref MemoryMarshal.GetReference(buffer), (nuint) buffer.Length);
}

`

New:

`
public static int GetManufacturerString(DeviceSafeHandle device, ReadOnlySpan buffer, int maxLength)
{
return GetManufacturerString(device, ref MemoryMarshal.GetReference(buffer), (nuint) maxLength);
}

public static int GetProductString(DeviceSafeHandle device, ReadOnlySpan<byte> buffer, int maxLength)
{
    return GetProductString(device, ref MemoryMarshal.GetReference(buffer), (nuint) maxLength);
}

public static int GetSerialNumberString(DeviceSafeHandle device, ReadOnlySpan<byte> buffer, int maxLength)
{
    return GetSerialNumberString(device, ref MemoryMarshal.GetReference(buffer), (nuint) maxLength);
}

public static int GetIndexedString(DeviceSafeHandle device, int index, ReadOnlySpan<byte> buffer, int maxLength)
{
    return GetIndexedString(device, index, ref MemoryMarshal.GetReference(buffer), (nuint) maxLength);
}

`

Additional Note

The string that is returned by this methods (after fixing the issue) contains a lot of '\0' characters (probably the hidapi sets the full memory area to '\0'). Maybe it would be nice if these '\0' are removed with something like .Trim('\0') before the string is returned.

Additional Note 2

As workaround I found ot that I could read the string s from the DeviceInfo by the method "GetDeviceInfo". That worked without issues.

[badcel]

Yes this sounds plausible.

In case your assumption is right increasing the buffersize should fix the issue, too as there would be enough memory for HidApi available. Can you confirm?

Edit: You can supply the buffer size via an optional parameter.

[AdrianEggenberger]

I tried that as well, but I did not fix anything. In the debugger I see that the full buffer given is written by the function. The required string is at the beginning and the rest is filled with '\0'. My asumption is that the method initializes the full buffer and threrefor writes everytime over the provided buffer.

[badcel]

Thanks for the clarification. I will see to create a fix.

[badcel]

@AdrianEggenberger: Thanks for your suggestions. It looks very promising.

During my tests I got unsure about the API. For my device the manufacturer is "Microsoft". If I supply a maxLength of 3 to HidApi only "Mi" will be returned with trailing zeros for the last character. So it seems the string ins null terminated inside the buffer.

This is a bit odd, but hiding this fact from the user is making the code a lot more complex so i opted to just document this behaviour.

See: #98

Can you confirm those changes are working like expected for you? Then I would merge the PR and create a release.

[AdrianEggenberger]

Just tested your changes and I can confirm it fixes the crashes.

As the hidapi API is a C/C++ API it makes sense that the provided buffer includes the terminating '\0' character. For the API in the .NET library I would personally hide this fact as .NET has no string terminator. The code in the methods of the Device.cs would then look like this:
var charBufferLength = maxLength + 1; // add one because of the trailing '\0' var buffer = new WCharTString(charBufferLength); var result = NativeMethods.GetManufacturerString(handle, buffer, charBufferLength);

At the end both ways are fine.

[badcel]

I thought that there could be problems if int.MaxValue is used as buffer size. But as it gets casted to nuint it could potentially be fine on 32 and 64Bit operating systems. I will evaluate again.

[badcel]

@AdrianEggenberger: I Updated the PullRequest one more time. Now it is hiding the null termination character from the public API. Additionally the possible OverflowException got documented. (It could be thrown in the old code already, as the number of needed bytes is at least two times bigger than the requested character size. The maximum byte array size is even smaller than int.MaxValue.)

As I changed the behaviour of the underlying wchar_t implementation (it allocates one character less than before) it would be nice if you could confirm that everything works like expected for you before I merge the pull request.

Thanks a lot for your valuable feedback.

[AdrianEggenberger]

Thanks for the update.

Tested with your latest commit. Everything works as expected.
I would say ready for merge.

[badcel]

Version 1.0.4 got released. Thank you!