Re: [PATCH 1/4] dlls/ntdll: Add handle count to NtQuerySystemInformation
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.
On 20 June 2011 15:08, Vitaliy Margolen <wine-devel(a)kievinfo.com> 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?
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.
On 20 June 2011 23:52, Vitaliy Margolen <wine-devel(a)kievinfo.com> 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.
On 06/20/2011 09:36 PM, Austin Lund wrote:
On 20 June 2011 23:52, Vitaliy Margolen<wine-devel(a)kievinfo.com> 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.
participants (2)
-
Austin Lund -
Vitaliy Margolen