user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

"Maarten Lankhorst" <m.b.lankhorst at gmail.com> wrote:

>  UINT WINAPI GetWindowModuleFileNameA( HWND hwnd, LPSTR lpszFileName, UINT cchFileNameMax)
>  {
> -    FIXME("GetWindowModuleFileNameA(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n",
> -          hwnd, lpszFileName, cchFileNameMax);
> -    return 0;
> +    LPWSTR filenameW = NULL;
> +    UINT retval;
> +
> +    if (cchFileNameMax)
> +    {
> +        filenameW = HeapAlloc(GetProcessHeap(), 0, cchFileNameMax);
> +        if (!filenameW)
> +        {
> +            SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> +            return 0;
> +        }
> +    }

Probably GetWindowModuleFileNameA should simply allocate a buffer of size
MAX_PATH on the stack and fail with ERROR_FILENAME_EXCED_RANGE if unicode
counterpart returns a string longer than MAX_PATH. kernel32 APIs behave
that way.

> +    retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax);
> +    if (retval)
> +    {
> +        DWORD lasterror = GetLastError();
> +        WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL);
> +        SetLastError(lasterror);

What's the point of saving/restoring last error value? If WideCharToMultiByte
fails you need to return an error in that case, not silently continue.

Also GetWindowModuleFileNameA also should be fixed to return correct value,
and not assume that the buffer is '\0' terminated.

>  UINT WINAPI GetWindowModuleFileNameW( HWND hwnd, LPWSTR lpszFileName, UINT cchFileNameMax)
>  {
> -    FIXME("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n",
> -          hwnd, lpszFileName, cchFileNameMax);
> -    return 0;
> +    PROCESS_BASIC_INFORMATION pbi;
> +    DWORD pid, retval;
> +    HANDLE hproc;
> +    DWORD lasterror = GetLastError();
> +    UNICODE_STRING *pathname;
> +    TRACE("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u)\n", hwnd, lpszFileName, 
> cchFileNameMax);

TRACE/WARN/ERR/etc already print the caller's name, there is no need to
duplicate it.

> +    if (!GetWindowThreadProcessId(hwnd, &pid))
> +    {
> +        SetLastError(ERROR_INVALID_WINDOW_HANDLE);
> +        return 0;
> +    }

GetWindowThreadProcessId already sets the error to the correct value,
there is no need to guess why it failed.

> +    if (!cchFileNameMax)
> +    { /* Fail silently without setting filename */
> +        SetLastError(lasterror);
> +        return 0;
> +    }

Saving/restoring last error value doesn't look correct at all.

> +    hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid);
> +    if (!hproc)
> +    {
> +        FIXME("Is this correct?");
> +        SetLastError(ERROR_INVALID_WINDOW_HANDLE);
> +        return 0;
> +    }

There is no need to guess why OpenProcess failed.

> +    retval = NtQueryInformationProcess(hproc, ProcessBasicInformation, &pbi, sizeof(pbi), NULL);
> +    pathname = &pbi.PebBaseAddress->ProcessParameters->ImagePathName;
> +    if (pathname->Length / sizeof(WCHAR) < cchFileNameMax)
> +    {
> +        memcpy(lpszFileName, pathname->Buffer, pathname->Length);
> +        lpszFileName[pathname->Length / sizeof(WCHAR)] = 0;
> +        retval = pathname->Length / sizeof(WCHAR) + 1;
> +    }
> +    else
> +    {
> +        memcpy(lpszFileName, pathname->Buffer, cchFileNameMax * sizeof(WCHAR));
> +        retval = cchFileNameMax;
> +    }

Both branches above should be merged, that will simplify the code. Just '\0'
terminate the result if there is enough space in the provided buffer.

> +    CloseHandle(hproc);
> +    SetLastError(lasterror);
> +    FIXME("--> %u %s\n", retval, debugstr_w(lpszFileName));

Left-over from debugging?


-- 
Dmitry.