Re: [PATCH] dinput: Limit axes remap and fix loop variable in the js driver

[Sebastian Lackner]

On 13.06.2017 08:06, Bruno Jesus wrote:
> Tested with the Xbox 360 controller, Wii Classic controler, PS2 controller + usb converter and a virtual device with 24 axes.
>
> Fixes https://bugs.winehq.org/show_bug.cgi?id=43120
> Signed-off-by: Bruno Jesus
>
> dlls/dinput/joystick_linux.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/dinput/joystick_linux.c b/dlls/dinput/joystick_linux.c
> index 1032659621..dea8f17324 100644
> --- a/dlls/dinput/joystick_linux.c
> +++ b/dlls/dinput/joystick_linux.c
> @@ -253,9 +253,11 @@ static INT find_joystick_devices(void)
>                  /* If no axes were configured but there are axes assume a 1-to-1 (wii controller) */
>                  if (joydev.axis_count && !found_axes)
>                  {
> -
>                int axes_limit = min(joydev.axis_count, 8); /* generic driver limit */

Is it intentional that you are ignoring any POV sticks here?
The limit of 8 only seems to apply to the buttons.
> -
>                ERR("Incoherent joystick data, advertised %d axes, detected 0. Assuming 1-to-1.\n",
>                     joydev.axis_count);
> -
>                for (j = 0; j < joydev.axis_count; j++)
> -
>                for (j = 0; j < axes_limit; j++)
>                    joydev.dev_axes_map[j] = j;
>            }
>        }
>
> @@ -472,7 +474,7 @@ static HRESULT alloc_device(REFGUID rguid, IDirectInputImpl *dinput,
>      df->dwNumObjs = newDevice->generic.devcaps.dwAxes + newDevice->generic.devcaps.dwPOVs + newDevice->generic.devcaps.dwButtons;
>      if (!(df->rgodf = HeapAlloc(GetProcessHeap(), 0, df->dwNumObjs * df->dwObjSize))) goto FAILED;
> - for (i = 0; i < newDevice->generic.device_axis_count; i++)
> - for (i = 0; i < newDevice->generic.devcaps.dwAxes; i++)
> {
>     int wine_obj = newDevice->generic.axis_map[i];
>
> -- 2.11.0

This change looks very suspicious because you are changing the
for-loop, but not the dwNumObjs counter above. You are allocating
dwPOVs additional entries which are now unused after this change.
Is this really correct, and did you test with a joystick with POV
switch?

Best regards,
Sebastian

[PATCH] dinput: Limit axes remap and fix loop variable in the js driver

[Bruno Jesus]

On Tue, Jun 13, 2017 at 5:50 AM, Sebastian Lackner
 wrote:
> > diff --git a/dlls/dinput/joystick_linux.c b/dlls/dinput/joystick_linux.c
> > index 1032659621..dea8f17324 100644
> > --- a/dlls/dinput/joystick_linux.c
> > +++ b/dlls/dinput/joystick_linux.c
> > @@ -253,9 +253,11 @@ static INT find_joystick_devices(void)
> >                  /* If no axes were configured but there are axes assume a 1-to-1 (wii controller) */
> >                  if (joydev.axis_count && !found_axes)
> >                  {
> > -
> >                int axes_limit = min(joydev.axis_count, 8); /* generic driver limit */
>
> Is it intentional that you are ignoring any POV sticks here?
> The limit of 8 only seems to apply to the buttons.

Thanks for the review. Yes, it is intentional. This code is very
specific to catch broken devices that should still work. Unfortunately
bogus devices like the one in the bug got caught in as backfire.
> > -
> >                ERR("Incoherent joystick data, advertised %d axes, detected 0. Assuming 1-to-1.\n",
> >                     joydev.axis_count);
> > -
> >                for (j = 0; j < joydev.axis_count; j++)
> > -
> >                for (j = 0; j < axes_limit; j++)
> >                    joydev.dev_axes_map[j] = j;
> >            }
> >        }
> >
> > @@ -472,7 +474,7 @@ static HRESULT alloc_device(REFGUID rguid, IDirectInputImpl *dinput,
> >      df->dwNumObjs = newDevice->generic.devcaps.dwAxes + newDevice->generic.devcaps.dwPOVs + newDevice->generic.devcaps.dwButtons;
> >      if (!(df->rgodf = HeapAlloc(GetProcessHeap(), 0, df->dwNumObjs * df->dwObjSize))) goto FAILED;
> > - for (i = 0; i < newDevice->generic.device_axis_count; i++)
> > - for (i = 0; i < newDevice->generic.devcaps.dwAxes; i++)
> > {
> >     int wine_obj = newDevice->generic.axis_map[i];
> >
> > -- 2.11.0
>
> This change looks very suspicious because you are changing the
> for-loop, but not the dwNumObjs counter above. You are allocating
> dwPOVs additional entries which are now unused after this change.
> Is this really correct, and did you test with a joystick with POV
> switch?

You are probably right and yes, I tested with the Xbox 360 controller
that has POV. I'm changing the patch to deal with the broken devices
in a single place and avoid changing ancient code.
> Best regards,
> Sebastian

Best regards,
Bruno