Re: [PATCH] gdb: trivial segfault fix in tui

[Hal Ashburner <hal@ashburner.info>]

On 15 August 2012 10:34, Doug Evans <dje@google.com> wrote:
>
> It seems tui is quite fragile with respect to window resizes.
> I was able to get a segv here for similar reasons, but I didn't write
> down the steps I did to trigger it. :-(
>
> tui-source.c:
>                         element->which_element.source.is_exec_point =
>                           (filename_cmp (((struct tui_win_element *)
>
> locator->content[0])->which_element.locator.file_name,
>                                          s->filename) == 0
>
> tui is also a bit, umm, weird.
>
> tui_free_window has this:
>
>       generic_win = tui_locator_win_info_ptr ();
>       if (generic_win != (struct tui_gen_win_info *) NULL)
>         {
>           tui_delete_win (generic_win->handle);
>           generic_win->handle = (WINDOW *) NULL;
>         }
>
> but tui_locator_win_info_ptr will never return NULL.
>
> struct tui_gen_win_info *
> tui_locator_win_info_ptr (void)
> {
>   return &_locator;
> }
>
> Blech!
>
> In the end I suspect there's a better fix, but I don't know tui well enough.
> I'd wait to see if someone else does.
>
> Also, I'd add a comment explaining *why* the test for content != NULL,
> and I'd rewrite it as:
>
>   return (TUI_SRC_WIN->generic.content_in_use
>               && tui_locator_win_info_ptr()->content != NULL
>               && ...);
>
> The style is more consistent this way.


Hi Doug,

Thanks for the review! :-)

Yeah it is "more consistent" the way you suggest, but is this what is
wanted in this particular case? Or do we want it to stand out a little
and attract our reader's attention, while being minimally intrusive to
the existing code?

There's a segfault, they're bad, they're annoying to have in the
middle of a debugging session because you lose all your debugging work
setting up context, yada yada I don't have to jibber about all that
here I'm sure.

The check for null fixes the segfaults that have bitten me. I
sometimes need to do a !reset to make C-X work again in my terminal
which triggers it less reliably, but not any more with the fix in.

TUI has some pretty hairy code as you rightly point out.
It appears to be not maintained quite as well as it might be. It does
have a particular use case I love which is less popular, single
stepping assembly language code so I would personally advocate keeping
tui in the gdb codebase.

My patch is a temporary fix until someone has time to look at the
entirety of how tui works and where it doesn't. Possibly re-factoring
and re-writing large parts of it. Now that might end up being me in
the absence of someone more familiar with the project already doing
it.

tui_win_info_ptr()->content is a pointer to a pointer. Which is being
used to store the address of an array of pointers to tui_gen_win_info.
It gets set to null on one of the window init routines.
tui_init_generic_part()
tui_resize_all() in tui_win.c calls tui_free_win() on all windows also
sets it to null so perhaps it's not being reallocated as implicitly
expected.

I'd be poking around there if it's me who gets time to delve deeper
into it and the proper formulation of the fix.

Note also that checking this thing for NULL is consistent with the
following line in the same file, so mine isn't the first time:
void
tui_vertical_source_scroll (enum tui_scroll_direction scroll_direction,
                int num_to_scroll)
{
  if (TUI_SRC_WIN->generic.content != NULL)


if (foo == NULL) is not a style I care for, it's tautological by way
of saying it twice. If this is how it's done in the gdb codebase then
obviously it's the right way.

So how about we express the patch as follows and get it in as it is
clearly and obviously better than what is there now. Then it can be
removed when something better and more stable comes along.
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 81c03ee..e1a080f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2012-08-15  Hal Ashburner <hal@ashburner.info>
+
+   * tui/tui-source.c: Check for null pointer to prevent segfault.
+
 2012-08-10  Sergio Durigan Junior  <sergiodj@redhat.com>
    * linespec.c (find_methods): Remove unused variables `i1' and
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 9ba9b1d..2126b36 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -334,11 +334,17 @@ tui_show_symtab_source (struct gdbarch *gdbarch,
struct symtab *s,
 int
 tui_source_is_displayed (char *fname)
 {
-  return (TUI_SRC_WIN->generic.content_in_use
-     && (filename_cmp (((struct tui_win_element *)
-                (tui_locator_win_info_ptr ())->
-                content[0])->which_element.locator.file_name,
-               fname) == 0));
+  if (tui_locator_win_info_ptr()->content == NULL) {
+    /* XXX FIXME corrects segfault, but why is content NULL
+     * after certian terminal resize operations? */
+    return 0;
+  } else {
+    return (TUI_SRC_WIN->generic.content_in_use
+        && (filename_cmp (((struct tui_win_element *)
+                   (tui_locator_win_info_ptr ())->
+                   content[0])->which_element.locator.file_name,
+                  fname) == 0));
+  }
 }


Thanks again,
Hal Ashburner