Re: [hatari-devel] Big bug in the SDL UI

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 24/06/2015 20:28, Nicolas Pomarède a écrit :

> This will do this part of the if in SDLGui_DoDialog
>          if (current_object >= 0 && dlg[current_object].type ==
> SGSCROLLBAR) {
>            ...
>          }
>
> But current_object refers to an object in main menu, while dlg not
> refers to the new dialog menu we want to draw (the "you need to reset"
> dialog), so this doesn't make sense.
>
> So I wonder, why does current_object keeps its value between different
> dialogs ? From what I see, it should be set to 0 (or -1) when a new
> dialog opens in SDLGui_DoDialog(), is there a case where we want
> current_object to be propagated between different dialog as it is now ?

This is now fixed, this bug was introduced long time ago when scrollbar 
clickable scrollbar was added to the SDL UI (in rev 2939 from 14 Sep 
2010 http://hg.tuxfamily.org/mercurialroot/hatari/hatari/rev/f0e07ee4d964)

When clicking on the scrollbar, dlgFileSelect.c will call 
SDLGui_DoDialog in a loop with the previous value of current_object.
The problem is that any dialog will also keep the previous value of 
current_object, so this line :

if (current_object >= 0 && dlg[current_object].type == SGSCROLLBAR) {

Is bad, bacause current_object can refer to a previous dialog while dlg 
is a new/different dialog ; so they can't be mixed (only in the case 
where the fileselector dialog is called in a loop).

Depending on the compiled code, dlg[current_object].type can be any 
value ; in my case it was SGSCROLLBAR, which explains why the "you need 
to reset dialog" was immediately closed and behaved as "cancel".

I added a parameter to SDLGui_DoDialog to clean or not current_object, 
this fixes the issue.

Nicolas

PS : I wanted to try "mudflap" with this to see if it detects the leak, 
but unfortunatelly mudflap was discontinued with latest gcc 4.9. It's 
replaced by "-fsanitize=address" 
(https://code.google.com/p/address-sanitizer/) , but after some quick 
tests it didn't seem to detect this case :(