Re: [hatari-devel] Joystick dialog crashes with Ubuntu 24.04

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Sat,  6 Jul 2024 06:05:03 +0000
schrieb Thomas Huth <th.huth@xxxxxxxxx>:

> Am Fri, 5 Jul 2024 17:00:06 +0300
> schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
> 
> > Hi,
> > 
> > User is reporting that opening SDL GUI Joystick dialog crashes Hatari, 
> > on Lubuntu 24.04:
> > 	https://www.atari-forum.com/viewtopic.php?p=466100
> > 
> > Is anybody here using some Ubuntu 24.04 variant?   And if yes, can you 
> > reproduce the issue?
> > 
> > 
> > I took a quick look at the related code, and it's possible that SDL 
> > Joystick name query may get given invalid struct address if SDL reports 
> > error or no joysticks, or user has invalid joystick mapping in his 
> > config file.
> > 
> > Attached patch will hopefully handle such issues.  Any comments on it?  
> 
> 
>  
>  /**
> - * Return maximum available real joystick ID
> + * Return maximum available real joystick ID, or
> + * zero on error or no joystick (to avoid invalid array accesses)
>   */
>  int Joy_GetMaxId(void)
>  {
>  	int count = SDL_NumJoysticks();
>  	if (count > JOYSTICK_COUNT)
>  		count = JOYSTICK_COUNT;
> -	return count - 1;
> +	if (count > 0)
> +		return count - 1;
> +	return 0;
>  }
> 
> That looks fishy ... how do you distinguish now between no joystick and one
> joystick? Both will return 0 now here.

Thinking about this twice, it's likely ok. We seem to be using ID 0 for
disabled joysticks all over the place, so this seems to be the right fix
for getting rid of the bad nJoyId == -1 in the configuration.

With regards to this hunk:

+	/* Disable invalid joystick mappings */
+	for (i = 0; i < JOYSTICK_COUNT; i++)
+	{
+		int joyid = ConfigureParams.Joysticks.Joy[i].nJoyId;
+		if (joyid < 0 || joyid >= JOYSTICK_COUNT)
+		{
+			/* otherwise it may result in invalid array access */
+			Log_Printf(LOG_WARN, "Selected real Joystick %d unavailable, disabling ST joystick %d\n", joyid, i);
+			ConfigureParams.Joysticks.Joy[i].nJoystickMode = JOYSTICK_DISABLED;
+			ConfigureParams.Joysticks.Joy[i].nJoyId = 0;
+		}
+	}

I think I'd change it to:

+	/* Disable invalid joystick mappings */
+	for (i = 0; i < JOYSTICK_COUNT; i++)
+	{
+		int joyid = ConfigureParams.Joysticks.Joy[i].nJoyId;
+		if (joyid < 0 || joyid >= JOYSTICK_COUNT)
+		{
+			if (ConfigureParams.Joysticks.Joy[i].nJoystickMode == JOYSTICK_REALSTICK)
+			{
+				Log_Printf(LOG_WARN, "Selected real Joystick %d unavailable, disabling ST joystick %d\n", joyid, i);
+				ConfigureParams.Joysticks.Joy[i].nJoystickMode = JOYSTICK_DISABLED;
+			}
+			/* otherwise it may result in invalid array access */
+			ConfigureParams.Joysticks.Joy[i].nJoyId = 0;
+		}
+	}

to avoid that you accidentally disable keyboard emulation where it is not
necessary.

With that change, you patch looks fine to me.

 Thomas