From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21050 invoked by alias); 16 Aug 2012 01:50:51 -0000 Received: (qmail 20849 invoked by uid 22791); 16 Aug 2012 01:50:49 -0000 X-SWARE-Spam-Status: No, hits=-4.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_EG X-Spam-Check-By: sourceware.org Received: from mail-we0-f169.google.com (HELO mail-we0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 16 Aug 2012 01:50:16 +0000 Received: by weys10 with SMTP id s10so1609373wey.0 for ; Wed, 15 Aug 2012 18:50:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.242.204 with SMTP id i54mr10120842wer.99.1345081814527; Wed, 15 Aug 2012 18:50:14 -0700 (PDT) Received: by 10.227.156.81 with HTTP; Wed, 15 Aug 2012 18:50:14 -0700 (PDT) In-Reply-To: References: Date: Thu, 16 Aug 2012 01:50:00 -0000 Message-ID: Subject: Re: [PATCH] gdb: trivial segfault fix in tui From: Hal Ashburner To: Doug Evans Cc: "Abid, Hafiz" , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00449.txt.bz2 On 15 August 2012 10:34, Doug Evans 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 + + * tui/tui-source.c: Check for null pointer to prevent segfault. + 2012-08-10 Sergio Durigan Junior * 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