[ros-kernel] NtUserBuildHwndList()

[Mike Nordell]

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?

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.

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.


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.

Did any of it make any sense?

/Mike