[ros-kernel] NtUserBuildHwndList()

[Royce Mitchell III]

Mike Nordell wrote:

>Royce Mitchell III wrote:
>
>  
>
>>Would someone please take a moment to see if this would be a
>>better/safer/(& more correct) implementation of NtUserBuildHwndList()?
>>    
>>
>
>A few comments:
>
>  
>
>>   if ( pWnd && dwCount < nBufSize )
>>-     pWnd[dwCount] = Child->Self;
>>+     Status = MmCopyToCaller(&pWnd[dwCount], &Child->Self, sizeof(HWND));
>>   dwCount++;
>>    
>>
>
>Since the pWnd parameter is not optional (it's at leats not marked as such),
>perhaps it could be checked at the very top of the function, since it's used
>by all three branches of it, and do an early "SetLastWin32Error(invalid
>param.); return 0;" if its missing?
>  
>
It's been a long time since I wrote the original version of this 
function, but if memory serves correctly, pWnd *is* optional, so that 
the callers can find out how much memory they need to allocate. However, 
I think ( or at I least I hope ) we implemented a strategy where we use 
a default buffer, and only call a 2nd time if the buffer wasn't big enough.

Since only our code calls this function, we could safely require the 
caller to use a default buffer, but I'd rather the function be able to 
work with a NULL pointer, so that if someone makes a change to one of 
the calling functions, it's less likely to break, and so a future 
malicious code-writer can't take the OS down.

>Then, since direct pointer access is no longer used to access pWnd, replaced
>by the safe MmCopyToCaller, those other checks if pWnd is something could be
>removed.
>  
>
If MmCopyToCaller() is supposed to be safe ( or is it? ), we could get 
rid of the if() statements anyway, since our code (should) never call it 
with NULL anyway.

>But this begs the question, is this function supposed to return the number
>of HWND's actually set in the array, or the total count (i.e. in case of
>overflow it should return the number required)? From the old code there was
>no real way of telling, since it would (if lucky) crash if the array was
>overflowed, but the patch makes it become the former.
>
>
>While the last change from direct pointer access to MmCopy* got braces (that
>I personaly think should be mandatory), the first didn't. I'd add them there
>too.
>  
>
The only reason I added braces where I did was to create a variable. 
That's just my personal coding style. If it's concensus that they all 
should, I can add them. No big deal. However, bear in mind that there 
has not be unanimity in our group on how to format code.

>Should my assumption be incorrect here, and pWnd indeed is optional, it
>should so be clearly marked both in the definition and declaration of the
>function, and most of my comments could be disregarded. However, then the
>handling of Status becomes possibly wrong, since the function would return
>number of HWND's set, not the size of the array actually required.
>  
>
Well... now that you mention it, I think it should be clearly documented 
in either case, so there's no future confusion on this :)