Re: [PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation

[Vitaliy Margolen]

On 06/19/2011 08:15 PM, Austin Lund wrote:
> -
>                ret = wine_server_call( req );
> -
>                shi->Count += reply->handles;
> -
>                len = sizeof(SYSTEM_HANDLE_ENTRY)*shi->Count + sizeof(ULONG);
> -
>                shi = RtlReAllocateHeap(GetProcessHeap(), HEAP_ZERO_MEMORY, shi, len);

Please do allocations outside of server call block. And add handling of 
failed realloc. Also please double the allocated size, don't reallocate 
after each server call.
> -
>                for (i = shi->Count - reply->handles; i<  shi->Count; i++) {
> -
>                    shi->Handle[i].OwnerPid = reply->pid;
> -
>                }

Please follow file's curly braces style - none for single line blocks, or on 
separate line for multi-line blocks.

Vitaliy.

[PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation

[Austin Lund]

On 20 June 2011 15:08, Vitaliy Margolen  wrote:
> On 06/19/2011 08:15 PM, Austin Lund wrote:
> > - ret = wine_server_call( req );
> > - shi->Count += reply->handles;
> > - len = sizeof(SYSTEM_HANDLE_ENTRY)*shi->Count +
> >
> > sizeof(ULONG);
> > - shi = RtlReAllocateHeap(GetProcessHeap(),
> >
> > HEAP_ZERO_MEMORY, shi, len);
>
> Please do allocations outside of server call block. And add handling of
> failed realloc. Also please double the allocated size, don't reallocate
> after each server call.
> > - for (i = shi->Count - reply->handles; i<  shi->Count;
> >
> > i++) {
> > - shi->Handle[i].OwnerPid = reply->pid;
> > - }
>
> Please follow file's curly braces style - none for single line blocks, or on
> separate line for multi-line blocks.

Thanks for the review.  Does this patch address these concerns correctly?

[PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation

[Vitaliy Margolen]

On 06/20/2011 05:28 AM, Austin Lund wrote:
> Thanks for the review.  Does this patch address these concerns correctly?

No quite there yet:
> -
>        SERVER_START_REQ( create_snapshot )

You not checking if this call succeeded or not.
> -
>            while (sizeof(ULONG) + sizeof(SYSTEM_HANDLE_ENTRY)*shi->Count >= len)

You don't need a loop to calculate new size.
> -
>                shi = RtlReAllocateHeap(GetProcessHeap(), HEAP_ZERO_MEMORY, shi, len);
> -
>                if (shi == NULL)
> -
>                    return STATUS_NO_MEMORY;

You leaking old shi here. Also why do you need to zero allocated memory if 
you assigning all of it valid values?

Vitaliy.

[PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation

[Austin Lund]

On 20 June 2011 23:52, Vitaliy Margolen  wrote:
> > - SERVER_START_REQ( create_snapshot )
>
> You not checking if this call succeeded or not.

I've tried to include all the checks in the new patch.
> > - while (sizeof(ULONG) +
> >
> > sizeof(SYSTEM_HANDLE_ENTRY)*shi->Count >= len)
>
> You don't need a loop to calculate new size.

I've done this a different way in the new patch which avoids the reallocs.
> You leaking old shi here. Also why do you need to zero allocated memory if
> you assigning all of it valid values?

Not all of the values are assigned.  Only the handle pid is accessible
with the current wineserver protocol.  The remainder of the fields are
best set to zero as this is a sane value for these fields to trigger
exceptions and figure out how/when these values are being accessed
(e.g. null pointers or null handles).  Later patches, which I have
already sent, try to fill in more of these fields.

[PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation

[Vitaliy Margolen]

On 06/20/2011 09:36 PM, Austin Lund wrote:
> On 20 June 2011 23:52, Vitaliy Margolen  wrote:
> > > -
> > >        SERVER_START_REQ( create_snapshot )
> >
> > You not checking if this call succeeded or not.
>
> I've tried to include all the checks in the new patch.
> > > -
> > >            while (sizeof(ULONG) +
> > >
> > > sizeof(SYSTEM_HANDLE_ENTRY)*shi->Count>= len)
> >
> > You don't need a loop to calculate new size.
>
> I've done this a different way in the new patch which avoids the reallocs.
> > You leaking old shi here. Also why do you need to zero allocated memory if
> > you assigning all of it valid values?
>
> Not all of the values are assigned.  Only the handle pid is accessible
> with the current wineserver protocol.  The remainder of the fields are
> best set to zero as this is a sane value for these fields to trigger
> exceptions and figure out how/when these values are being accessed
> (e.g. null pointers or null handles).  Later patches, which I have
> already sent, try to fill in more of these fields.

Makes sense. The patch looks good to me.

Vitaliy.