Re: [ros-dev] [ros-diffs] [cgutman] 53232: [NTOSKRNL] - Fix NULL termination of strings in IoGetDeviceProperty - Fixes garbage displayed in the Enumerator field of the device manager property page

[Alex Ionescu]

And, you have, of course, confirmed that this piece of kernel code is wrong,
and that DevMgr/Cfgapi/SetupApi are doing the right thing, right?

So if I was to reverse engineer ntoskrnl I wouldn't discover that it was
actually doing the same thing as the un-"fixed" code, right?

Best regards,
Alex Ionescu


On Sun, Aug 14, 2011 at 10:44 AM, <cgutman(a)svn.reactos.org> wrote:

>   Author: cgutman
>  Date: Sun Aug 14 14:44:34 2011
>  New Revision: 53232
>
>  URL: http://svn.reactos.org/svn/reactos?rev=53232&view=rev
>  Log:
>  [NTOSKRNL]
>  - Fix NULL termination of strings in IoGetDeviceProperty
>  - Fixes garbage displayed in the Enumerator field of the device manager
>  property page
>
>  Modified:
>     trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c
>
>  Modified: trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c [iso-8859-1] Sun Aug 14
>  14:44:34 2011
>  @@ -3467,6 +3467,8 @@
>      NTSTATUS Status = STATUS_BUFFER_TOO_SMALL;
>      GUID BusTypeGuid;
>      POBJECT_NAME_INFORMATION ObjectNameInfo = NULL;
>  +    BOOLEAN NullTerminate = FALSE;
>  +
>      DPRINT("IoGetDeviceProperty(0x%p %d)\n", DeviceObject, DeviceProperty);
>
>      /* Assume failure */
>  @@ -3517,7 +3519,10 @@
>              /* Get the name from the path */
>              EnumeratorNameEnd = wcschr(DeviceInstanceName,
>  OBJ_NAME_PATH_SEPARATOR);
>              ASSERT(EnumeratorNameEnd);
>  -
>  +
>  +            /* This string needs to be NULL-terminated */
>  +            NullTerminate = TRUE;
>  +
>              /* This is the format of the returned data */
>              PIP_RETURN_DATA((EnumeratorNameEnd - DeviceInstanceName) *
>  sizeof(WCHAR),
>                              DeviceInstanceName);
>  @@ -3567,7 +3572,10 @@
>                  /* It's up to the caller to try again */
>                  Status = STATUS_BUFFER_TOO_SMALL;
>              }
>  -
>  +
>  +            /* This string needs to be NULL-terminated */
>  +            NullTerminate = TRUE;
>  +
>              /* Return if successful */
>              if (NT_SUCCESS(Status))
>  PIP_RETURN_DATA(ObjectNameInfo->Name.Length,
>
>  ObjectNameInfo->Name.Buffer);
>  @@ -3633,15 +3641,14 @@
>      else if (NT_SUCCESS(Status))
>      {
>          /* We know up-front how much data to expect, check the caller's
>  buffer */
>  -        *ResultLength = ReturnLength;
>  -        if (ReturnLength <= BufferLength)
>  +        *ResultLength = ReturnLength + (NullTerminate ?
>  sizeof(UNICODE_NULL) : 0);
>  +        if (*ResultLength <= BufferLength)
>          {
>              /* Buffer is all good, copy the data */
>              RtlCopyMemory(PropertyBuffer, Data, ReturnLength);
>
>  -            /* Check for properties that require a null-terminated string
>  */
>  -            if ((DeviceProperty == DevicePropertyEnumeratorName) ||
>  -                (DeviceProperty ==
>  DevicePropertyPhysicalDeviceObjectName))
>  +            /* Check if we need to NULL-terminate the string */
>  +            if (NullTerminate)
>              {
>                  /* Terminate the string */
>                  ((PWCHAR)PropertyBuffer)[ReturnLength / sizeof(WCHAR)] =
>  UNICODE_NULL;
>
>
>

[Cameron Gutman]

Well, writing beyond the buffer length we were supplied to null terminate a string seems
bad in my book ;)

Cameron

On Aug 14, 2011, at 11:00 AM, Alex Ionescu <ionucu(a)videotron.ca> wrote:

>   And, you have, of course, confirmed that this piece of
> kernel code is wrong, and that DevMgr/Cfgapi/SetupApi are doing the right thing, right?
>  
>  So if I was to reverse engineer ntoskrnl I wouldn't discover that it was actually
> doing the same thing as the un-"fixed" code, right?
>  
>  Best regards,
>  Alex Ionescu
>  
>  
>  On Sun, Aug 14, 2011 at 10:44 AM, <cgutman(a)svn.reactos.org> wrote:
>  Author: cgutman
>  Date: Sun Aug 14 14:44:34 2011
>  New Revision: 53232
>  
>  URL: http://svn.reactos.org/svn/reactos?rev=53232&view=rev
>  Log:
>  [NTOSKRNL]
>  - Fix NULL termination of strings in IoGetDeviceProperty
>  - Fixes garbage displayed in the Enumerator field of the device manager property page
>  
>  Modified:
>     trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c
>  
>  Modified: trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/io/pnpmgr/pnpmgr.c [iso-8859-1] Sun Aug 14 14:44:34 2011
>  @@ -3467,6 +3467,8 @@
>      NTSTATUS Status = STATUS_BUFFER_TOO_SMALL;
>      GUID BusTypeGuid;
>      POBJECT_NAME_INFORMATION ObjectNameInfo = NULL;
>  +    BOOLEAN NullTerminate = FALSE;
>  +
>      DPRINT("IoGetDeviceProperty(0x%p %d)\n", DeviceObject, DeviceProperty);
>  
>      /* Assume failure */
>  @@ -3517,7 +3519,10 @@
>              /* Get the name from the path */
>              EnumeratorNameEnd = wcschr(DeviceInstanceName, OBJ_NAME_PATH_SEPARATOR);
>              ASSERT(EnumeratorNameEnd);
>  -
>  +
>  +            /* This string needs to be NULL-terminated */
>  +            NullTerminate = TRUE;
>  +
>              /* This is the format of the returned data */
>              PIP_RETURN_DATA((EnumeratorNameEnd - DeviceInstanceName) * sizeof(WCHAR),
>                              DeviceInstanceName);
>  @@ -3567,7 +3572,10 @@
>                  /* It's up to the caller to try again */
>                  Status = STATUS_BUFFER_TOO_SMALL;
>              }
>  -
>  +
>  +            /* This string needs to be NULL-terminated */
>  +            NullTerminate = TRUE;
>  +
>              /* Return if successful */
>              if (NT_SUCCESS(Status)) PIP_RETURN_DATA(ObjectNameInfo->Name.Length,
>                                                      ObjectNameInfo->Name.Buffer);
>  @@ -3633,15 +3641,14 @@
>      else if (NT_SUCCESS(Status))
>      {
>          /* We know up-front how much data to expect, check the caller's buffer */
>  -        *ResultLength = ReturnLength;
>  -        if (ReturnLength <= BufferLength)
>  +        *ResultLength = ReturnLength + (NullTerminate ? sizeof(UNICODE_NULL) : 0);
>  +        if (*ResultLength <= BufferLength)
>          {
>              /* Buffer is all good, copy the data */
>              RtlCopyMemory(PropertyBuffer, Data, ReturnLength);
>  
>  -            /* Check for properties that require a null-terminated string */
>  -            if ((DeviceProperty == DevicePropertyEnumeratorName) ||
>  -                (DeviceProperty == DevicePropertyPhysicalDeviceObjectName))
>  +            /* Check if we need to NULL-terminate the string */
>  +            if (NullTerminate)
>              {
>                  /* Terminate the string */
>                  ((PWCHAR)PropertyBuffer)[ReturnLength / sizeof(WCHAR)] = UNICODE_NULL;
>  
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Timo Kreuzer]

Duty Calls

:D

Am 14.08.2011 17:00, schrieb Alex Ionescu:
>   And, you have, of course, confirmed that this piece of
> kernel code is 
>  wrong, and that DevMgr/Cfgapi/SetupApi are doing the right thing, right?
>
>  So if I was to reverse engineer ntoskrnl I wouldn't discover that it 
>  was actually doing the same thing as the un-"fixed" code, right?
>
>  Best regards,
>  Alex Ionescu