Re: dinput: Implement device property DIPROP_USERNAME.

[Sebastian Lackner]

On 01.12.2015 14:54, Bernhard Übelacker wrote:
> https://bugs.winehq.org/show_bug.cgi?id=39667
>
> Probably same issue as in https://bugs.winehq.org/show_bug.cgi?id=12432 .
> (Attached backtrace seems equal.)
>
> Steps to reproduce:
> - start launcher
> - "Configure Controller"
> - leave dialog with "Cancel"
> - crash
>
> MotoGP 3 demo launcher uses ConfigureDevices for the key mapping.
> The crash seems to happen because the result of a
> GetProperty(DIPROP_USERNAME) is used without checking.
>
> This affects just the launcher. The game starts without problems with
> a default key mapping.
> Signed-off-by: Bernhard Übelacker
>
> dlls/dinput/device.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  dlls/dinput/device_private.h |  1 +
>  dlls/dinput8/tests/device.c  | 29 +++++++++++++++++++++++++++--
>  3 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c
> index e525f01..1095c4c 100644
> --- a/dlls/dinput/device.c
> +++ b/dlls/dinput/device.c
> @@ -783,6 +783,7 @@ HRESULT _set_action_map(LPDIRECTINPUTDEVICE8W iface, LPDIACTIONFORMATW lpdiaf, L
>      DIOBJECTDATAFORMAT *obj_df = NULL;
>      DIPROPDWORD dp;
>      DIPROPRANGE dpr;
> - DIPROPSTRING dps;
> WCHAR username[MAX_PATH];
> DWORD username_size = MAX_PATH;
> int i, action = 0, num_actions = 0;
>
> @@ -863,6 +864,19 @@ HRESULT _set_action_map(LPDIRECTINPUTDEVICE8W iface, LPDIACTIONFORMATW lpdiaf, L
>      else
>          lstrcpynW(username, lpszUserName, MAX_PATH);
> - dps.diph.dwSize = sizeof(dps);
> - dps.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> - dps.diph.dwObj = 0;
> - dps.diph.dwHow  = DIPH_DEVICE;
> - lstrcpynW(dps.wsz, username, sizeof(dps.wsz));
> - IDirectInputDevice8_SetProperty(iface, DIPROP_USERNAME, &dps.diph);
> -
> - if (dwFlags & DIDSAM_NOUSER)
> - {
> -
>    HeapFree(GetProcessHeap(), 0, This->username);
> -
>    This->username = NULL;
> - }

When I understand it correctly, if DISAM_NOUSER is set, you set the
username just to reset it manually again afterwards? Why not pass an
empty string to IDirectInputDevice8_SetProperty?
> - /* Save the settings to disk */
>    save_mapping_settings(iface, lpdiaf, username);
>
> @@ -1100,6 +1114,9 @@ ULONG WINAPI IDirectInputDevice2WImpl_Release(LPDIRECTINPUTDEVICE8W iface)
>      /* Free action mapping */
>      HeapFree(GetProcessHeap(), 0, This->action_map);
> - /* Free username */
> - HeapFree(GetProcessHeap(), 0, This->username);
> - EnterCriticalSection( &This->dinput->crit );
>    list_remove( &This->entry );
>    LeaveCriticalSection( &This->dinput->crit );
>
> @@ -1251,6 +1268,17 @@ HRESULT WINAPI IDirectInputDevice2WImpl_GetProperty(LPDIRECTINPUTDEVICE8W iface,
>              TRACE("buffersize = %d\n", pd->dwData);
>              break;
>          }
> -
>    case (DWORD_PTR) DIPROP_USERNAME:
> -
>    {
> -
>        LPDIPROPSTRING ps = (LPDIPROPSTRING)pdiph;
> -
> -
>        if (pdiph->dwSize != sizeof(DIPROPSTRING)) return DIERR_INVALIDPARAM;
> -
> -
>        ps->wsz[0] = 0;
> -
>        if (This->username)
> -
>            lstrcpynW(ps->wsz, This->username, sizeof(ps->wsz));

The last argument is wrong, it should be a WCHAR count.
> -
>        break;
> -
>    }
>    case (DWORD_PTR) DIPROP_VIDPID:
>        FIXME("DIPROP_VIDPID not implemented\n");
>        return DIERR_UNSUPPORTED;
>
> @@ -1324,6 +1352,22 @@ HRESULT WINAPI IDirectInputDevice2WImpl_SetProperty(
>              LeaveCriticalSection(&This->crit);
>              break;
>          }
> -
>    case (DWORD_PTR) DIPROP_USERNAME:
> -
>    {
> -
>        LPCDIPROPSTRING ps = (LPCDIPROPSTRING)pdiph;
> -
> -
>        if (pdiph->dwSize != sizeof(DIPROPSTRING)) return DIERR_INVALIDPARAM;
> -
> -
>        if (!This->username)
> -
>            This->username = HeapAlloc(GetProcessHeap(), 0, sizeof(ps->wsz));
> -
>        if (!This->username)
> -
>            return DIERR_OUTOFMEMORY;
> -
> -
>        This->username[0] = 0;
> -
>        if (ps->wsz)
> -
>            lstrcpynW(This->username, ps->wsz, sizeof(ps->wsz));

Same here. Also, it could make sense to look at the actual string length, instead
of allocating a fixed size buffer.
> -
>        break;
> -
>    }
>    default:
>        WARN("Unknown property %s\n", debugstr_guid(rguid));
>        return DIERR_UNSUPPORTED;
>
> diff --git a/dlls/dinput/device_private.h b/dlls/dinput/device_private.h
> index 52bbec4..26ec7c2 100644
> --- a/dlls/dinput/device_private.h
> +++ b/dlls/dinput/device_private.h
> @@ -81,6 +81,7 @@ struct IDirectInputDeviceImpl
>      /* Action mapping */
>      int                         num_actions; /* number of actions mapped */
>      ActionMap                  *action_map;  /* array of mappings */
> - WCHAR                      *username;    /* set in SetActionMap */
>
> };
>
> extern BOOL get_app_key(HKEY*, HKEY*) DECLSPEC_HIDDEN;
> diff --git a/dlls/dinput8/tests/device.c b/dlls/dinput8/tests/device.c
> index 6495559..af8667d 100644
> --- a/dlls/dinput8/tests/device.c
> +++ b/dlls/dinput8/tests/device.c
> @@ -223,8 +223,8 @@ static BOOL CALLBACK enumeration_callback(const DIDEVICEINSTANCEA *lpddi, IDirec
>      dps.wsz[0] = '\0';
>  hr = IDirectInputDevice_GetProperty(lpdid, DIPROP_USERNAME, &dps.diph);
> - todo_wine ok (SUCCEEDED(hr), "GetProperty failed hr=%08x\n", hr);
> - todo_wine ok (!lstrcmpW(usernameW, dps.wsz), "Username not set correctly expected=%s, got=%s\n", wine_dbgstr_wn(usernameW, -1), wine_dbgstr_wn(dps.wsz, -1));
> -
>
> ok (SUCCEEDED(hr), "GetProperty failed hr=%08x\n", hr);
> -
>
> ok (!lstrcmpW(usernameW, dps.wsz), "Username not set correctly expected=%s, got=%s\n", wine_dbgstr_wn(usernameW, -1), wine_dbgstr_wn(dps.wsz, -1));
>
> /* Test buffer size */
> memset(&dp, 0, sizeof(dp));
>
> @@ -275,6 +275,7 @@ static void test_action_mapping(void)
>      HINSTANCE hinst = GetModuleHandleA(NULL);
>      IDirectInput8A *pDI = NULL;
>      DIACTIONFORMATA af;
> - DIPROPSTRING dps;
> struct enum_data data =  {pDI, &af, NULL, NULL, NULL, 0};
> HWND hwnd;
>
> @@ -342,6 +343,30 @@ static void test_action_mapping(void)
>      af.dwDataSize = 4 * sizeof(actionMapping) / sizeof(actionMapping[0]);
>      af.dwNumActions = sizeof(actionMapping) / sizeof(actionMapping[0]);
> -
> -
>    /* test DIDSAM_NOUSER */
> -
>    dps.diph.dwSize = sizeof(dps);
> -
>    dps.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> -
>    dps.diph.dwObj = 0;
> -
>    dps.diph.dwHow  = DIPH_DEVICE;
> -
>    dps.wsz[0] = '\0';
> -
> -
>    hr = IDirectInputDevice_GetProperty(data.keyboard, DIPROP_USERNAME, &dps.diph);
> -
>    ok (SUCCEEDED(hr), "GetProperty failed hr=%08x\n", hr);
> -
>    ok (dps.wsz[0] != 0, "Expected any username, got=%s\n", wine_dbgstr_wn(dps.wsz, -1));
> -
> -
>    hr = IDirectInputDevice8_SetActionMap(data.keyboard, data.lpdiaf, NULL, DIDSAM_NOUSER);
> -
>    ok (SUCCEEDED(hr), "SetActionMap failed hr=%08x\n", hr);
> -
> -
>    dps.diph.dwSize = sizeof(dps);
> -
>    dps.diph.dwHeaderSize = sizeof(DIPROPHEADER);
> -
>    dps.diph.dwObj = 0;
> -
>    dps.diph.dwHow  = DIPH_DEVICE;
> -
>    dps.wsz[0] = '\0';
> -
> -
>    hr = IDirectInputDevice_GetProperty(data.keyboard, DIPROP_USERNAME, &dps.diph);
> -
>    ok (SUCCEEDED(hr), "GetProperty failed hr=%08x\n", hr);
> -
>    ok (dps.wsz[0] == 0, "Expected empty username, got=%s\n", wine_dbgstr_wn(dps.wsz, -1));
>
> }
>
> if (data.mouse != NULL)