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

[Doug Evans <dje@google.com>]

On Wed, Aug 15, 2012 at 6:50 PM, Hal Ashburner <hal@ashburner.info> wrote:
> On 15 August 2012 10:34, Doug Evans <dje@google.com> wrote:
> > [...]
> 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

I like this alternative patch better:
http://sourceware.org/ml/gdb-patches/2012-08/msg00809.html

So let's go with that.