Re: [PATCH v2] ntoskrnl.exe: Implement DevicePropertyEnumeratorName for IoGetDeviceProperty

[Sebastian Lackner]

On 14.09.2016 18:06, Aric Stewart wrote:
> v2: Cleanup and resend
> Signed-off-by: Aric Stewart
>
> dlls/ntoskrnl.exe/ntoskrnl.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> v2-0001-ntoskrnl.exe-Implement-DevicePropertyEnumeratorName.txt
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> index aebb3de..d1b0ee9 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> @@ -94,6 +94,8 @@ struct wine_driver
>      DRIVER_EXTENSION driver_extension;
>  };
>
> +static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id );
> -
>
> static int wine_drivers_rb_compare( const void *key, const struct wine_rb_entry *entry )
>  {
>      const struct wine_driver *driver = WINE_RB_ENTRY_VALUE( entry, const struct wine_driver, entry );
> @@ -1132,6 +1134,34 @@ NTSTATUS WINAPI IoGetDeviceProperty( DEVICE_OBJECT *device, DEVICE_REGISTRY_PROP
>             property_buffer, result_length );
>      switch (device_property)
>      {
> -
>    case DevicePropertyEnumeratorName:
> -
>    {
> -
>        DWORD len;
> -
>        WCHAR *id, *ptr;
> -
>        status = get_device_id( device, BusQueryInstanceID, &id );
> -
>        if (status != STATUS_SUCCESS)
> -
>        {
> -
>            ERR( "Failed to get device id\n" );
> -
>            break;
> -
>        }
> -
>        struprW( id );
> -
>        TRACE( "Device ID %s\n",debugstr_w( id ) );

I am not sure if the TRACE is very useful here. Its not the value returned
by get_device_id() and not the result returned by this function. Does it
really make sense to dump this intermediate value here?
> -
>        ptr = strchrW( id, '\\' );
> -
>        if (ptr)
> -
>            len = (ptr - id);

It would be easier to replace the '\' with a null character, and then
always use strlenW() to get the length. This also allows to simplify the
code below.
> -
>        else
> -
>            len = strlenW( id );
> -
>        *result_length = sizeof( WCHAR ) * (len + 1);
> -
>        if (buffer_length < *result_length)
> -
>        {
> -
>            status = STATUS_BUFFER_TOO_SMALL;

You are leaking the id buffer here and in the return path below.
> -
>            break;
> -
>        }
> -
> -
>        lstrcpynW( property_buffer, id, len + 1 );
> -
>        ((WCHAR*)property_buffer)[len] = 0;

When the string is properly null terminated you could just use memcpy()
here and copy *result_length bytes.
> -
>        break;
> -
>    }
>    case DevicePropertyPhysicalDeviceObjectName:
>    {
>        ULONG used_len, len = buffer_length + sizeof(OBJECT_NAME_INFORMATION);