Re: [Bug-gnubg] Tutor mode patches for gnubg

[Jim Segrave]

This is a rather lengthy analysis of the problem which I attempted to
patch in gtkgame.c DestroyHint(). As it turns out, the patch helps
prevent core dumps, but does not address the real issue, which is that 
when gtk is tearing down the Hint window, the hintdata associated with
that window is being freed before DestroyHint() gets to use it to find
and free up the movelist:

I did some testing of the Hint window handling, after verifying that I
could get core dumps both with and without the tutor code, so I didn't
add anything which broke things.

First I added a couple of asserts in DetroyHint() and a little code to
make debugging easier at the end of GTKHint():

static void DestroyHint( gpointer p ) {
    hintdata *phd = gtk_object_get_user_data( GTK_OBJECT( pwHint ) );

        assert (phd != NULL);
=>  assert (phd->pml != NULL);

        if (phd->pml) {
          if (phd->pml->amMoves)
                free( phd->pml->amMoves );

          free( phd->pml );
    }

    pwHint = NULL;
}

static movelist *pmovelist_save;
static hintdata *phintdata_save, hintdata_save;

extern void GTKHint( movelist *pmlOrig ) {
    GtkWidget *pwButtons,
        *pwMove = gtk_button_new_with_label( _("Move") ),
#ifdef WIN32
[snip]
    gtk_widget_show_all( pwHint );
        pmovelist_save = pml;
        phintdata_save = phd;
        memcpy ((char *)&hintdata_save, (char *)phd, sizeof (hintdata));
        if (pmovelist_save->amMoves == 0)
          *(int *)(pmovelist_save->amMoves) = 1;

}

This keeps a copy of the hintdata and the values of the pointers set
up by GTKHint() so I can look at them later undr gdv.

I then run gnubg, asking for a hint after every roll and again after
having made a move, just before clicking the dice to get gnubg to
play. After a few moves - the earliest I have seen is the second move,
other times it has taken up to 10 or more moves, the assert is
triggered:

In gdb, I looked at what had happened:

#3  0x80af574 in DestroyHint (p=0x81c9d98) at gtkgame.c:4599

This is the value of phd->pml when GTKHint() completed
(gdb) p pmovelist_save
$1 = (movelist *) 0x8acd180

Here's where the hintdata is supposed to be when DestroyHint() runs
(gdb) p phd
$2 = (hintdata *) 0x828d7c0

And here's what it was when GTKHint() completed - it's hasn't moved
(gdb) p phintdata_save
$3 = (hintdata *) 0x828d7c0

Here's what the hintdata looked like when GTKHint completed
(gdb) p hintdata_save
$4 = {pwMove = 0x8addd20, pw = 0x8adba00, pwRollout = 0x8afe070, 
  pwRolloutSettings = 0x8afe018, pwEval = 0x82ba330, 
  pwEvalSettings = 0x82ba388, pml = 0x8acd180, fButtonsValid = 1, 
  piHighlight = 0x0}

But now it's been overwritten with junk:

(gdb) p *phd
$5 = {pwMove = 0x0, pw = 0x0, pwRollout = 0x285f1ff0, pwRolloutSettings = 0x0, 
  pwEval = 0x8247980, pwEvalSettings = 0xc8, pml = 0x0, fButtonsValid = 271, 
  piHighlight = 0x0}

I then used gdb to set a hardware breakpoint at the end of GTKHint()
looking for writes to the beginning of the hintdata and a breakpoint
in DestroyHint() so I could delete the hardware watchpoint
afterwards. What I found was that gtk window management operations are
re-using the user data allocated to the Hint window. The backtrace
when the user data is being overwritten is quite long (it's 126 frames
deep). But at frame 92, it's in OK() where it calls
gtk_widget_destroy( gtk_widget_get_toplevel( pw ) );

The actual value called in gtk_widget_destroy (found by dumping the
stack and locating the return address and pushed value) is pwHint.

So it is calling gtk_widget_destroy (pwHint). The backtrace shows gnubg
stepping through one of the containers, deleting each object
in the container.

The most interesting point is that the hardware breakpoint, triggered
by overwriting the start of the hintdata in the user data part of
pwHint occurs when bzero() is called  from gmalloc0(), which I assume is
a calloc type call. 

The area bzero is being asked to clear is precisely the address of the
user data. So it appears to me that gmalloc0 has re-issued this
same block of memory which suggests that it has somehow been freed
between the time the Hint window was created and this point when
processing a mouse click on the OK button. If the addresses differed
but happened to overlap the hintdata, then I'd be suspecting memory
heap corruption, but this looks like normal re-allocation of a block
of memory which has been freed.

I think the differences between the Linux malloc and the FreeBSD one
may explain why this is not seen as often on Linux systems. The
likelihood of the hintdata being freed and the memory used for holding
it being re-issued is lower than with the FreeBSD malloc, so most of
the time the block will still be intact when DestroyHint() runs and it
can find the movelist pointer and free the memory. As I understand it,
the FreeBSD malloc has pools of various sized blocks of memory. If you
malloc a hintdata block (34 bytes), then free it, then it will
probably be the first block handed out on the next malloc request for
blocks between 32 and 64 bytes in length.

It's my belief that  DestroyHint() needs to be run before some of the other
widget_destroy gtk functions get run, so that the movelist data is
still available.

I don't claim to be too au fait with the internals of gtk, but it was
my impression that setting up a reference should preserve the
data. GTKHint() does do a gtk_object_weakref on pwHint. The html
reference manual suggests that this probably isn't strong enough and
this should be a gtk_object_ref instead. If I understand the docs, the
weakref is enough to get DestroyHint called, but does not ensure that
the data associated with pwHint will still be valid.

But I'd like someone with a better knowledge of gtk to offer an
opinion on this one. 

What I am sure of is that DestroyHint may well be called and receive a
pointer to the hintdata even though the memory holding the hintdata
may well have been re-used. Checking for phd->pml != NULL is not
really sufficient - it's only luck that the re-use of this block
hasn't put some other non-zero data in this element.

-- 
Jim Segrave           address@hidden