Re: ntdll: Fix RtlIntegerToUnicodeString so it won't overflow

[Alexandre Julliard]

"Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> writes:
> @@ -1970,7 +1970,7 @@ NTSTATUS WINAPI RtlIntegerToUnicodeString(
>      } while (value != 0L);
>
> str->Length = (&buffer[32] - pos) * sizeof(WCHAR);
> -    if (str->Length >= str->MaximumLength) {
> +    if (str->Length + sizeof(WCHAR) >= str->MaximumLength) {
>   return STATUS_BUFFER_OVERFLOW;
>      } else {
>   memcpy(str->Buffer, pos, str->Length + sizeof(WCHAR));

There's no overflow here. The Windows implementation of
RtlIntegerToUnicodeString seems badly confused but I don't think
we need to replicate those bugs.

-- 
Alexandre Julliard
julliard(a)winehq.org

ntdll: Fix RtlIntegerToUnicodeString so it won't overflow

[Maarten Lankhorst]

Hello Alexandre,

2008/5/8 Alexandre Julliard <julliard(a)winehq.org>:
> "Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> writes:
> > @@ -1970,7 +1970,7 @@ NTSTATUS WINAPI RtlIntegerToUnicodeString(
> >      } while (value != 0L);
> >
> > str->Length = (&buffer[32] - pos) * sizeof(WCHAR);
> > -    if (str->Length >= str->MaximumLength) {
> > +    if (str->Length + sizeof(WCHAR) >= str->MaximumLength) {
> >       return STATUS_BUFFER_OVERFLOW;
> >      } else {
> >       memcpy(str->Buffer, pos, str->Length + sizeof(WCHAR));
>
> There's no overflow here. The Windows implementation of
>  RtlIntegerToUnicodeString seems badly confused but I don't think
>  we need to replicate those bugs.

It copies str->Length + sizeof(WCHAR) to the destination buffer
according to james' testcases. So it definitely looks like a bug to me
if it would copy data beyond MaximumLength, since only up to
MaximumLength is guaranteed to be allocated. Of course you're right
that my fix is likely wrong, the >= max should probablly be changed to
> max, otherwise it would return STATUS_BUFFER_OVERFLOW wrongly.

Cheers,
Maarten.

ntdll: Fix RtlIntegerToUnicodeString so it won't overflow

[James Hawkins]

On Thu, May 8, 2008 at 1:00 PM, Maarten Lankhorst
<m.b.lankhorst(a)gmail.com> wrote:
> Hello Alexandre,
>
> 2008/5/8 Alexandre Julliard <julliard(a)winehq.org>:
> > "Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> writes:
> > > @@ -1970,7 +1970,7 @@ NTSTATUS WINAPI RtlIntegerToUnicodeString(
> > >      } while (value != 0L);
> > >
> > > str->Length = (&buffer[32] - pos) * sizeof(WCHAR);
> > > -    if (str->Length >= str->MaximumLength) {
> > > +    if (str->Length + sizeof(WCHAR) >= str->MaximumLength) {
> > >       return STATUS_BUFFER_OVERFLOW;
> > >      } else {
> > >       memcpy(str->Buffer, pos, str->Length + sizeof(WCHAR));
> >
> > There's no overflow here. The Windows implementation of
> >  RtlIntegerToUnicodeString seems badly confused but I don't think
> >  we need to replicate those bugs.
>
> It copies str->Length + sizeof(WCHAR) to the destination buffer
> according to james' testcases.

No, the length is indeterminate.
> So it definitely looks like a bugto me
> if it would copy data beyond MaximumLength, since only up to
> MaximumLength is guaranteed to be allocated. Of course you're right
> that my fix is likely wrong, the >= max should probablly be changed to
> > max, otherwise it would return STATUS_BUFFER_OVERFLOW wrongly.
>
> Cheers,
> Maarten.

-- 
James Hawkins

ntdll: Fix RtlIntegerToUnicodeString so it won't overflow

[Alexandre Julliard]

"Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> writes:
> It copies str->Length + sizeof(WCHAR) to the destination buffer
> according to james' testcases. So it definitely looks like a bug to me
> if it would copy data beyond MaximumLength, since only up to
> MaximumLength is guaranteed to be allocated. Of course you're right
> that my fix is likely wrong, the >= max should probablly be changed to
> > max, otherwise it would return STATUS_BUFFER_OVERFLOW wrongly.

And that's exactly what the existing code is doing (except if
MaximumLength is odd but that makes no sense for a WCHAR buffer).

-- 
Alexandre Julliard
julliard(a)winehq.org