* Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed() [not found] <20200106102649.15710-1-shahab.vahedi@gmail.com> @ 2020-01-06 12:18 ` Pedro Alves 2020-01-06 12:43 ` Shahab Vahedi 0 siblings, 1 reply; 5+ messages in thread From: Pedro Alves @ 2020-01-06 12:18 UTC (permalink / raw) To: Shahab Vahedi, gdb-patches Cc: Shahab Vahedi, Claudiu Zissulescu, Francois Bedard On 1/6/20 10:26 AM, Shahab Vahedi wrote: > From: Shahab Vahedi <shahab@synopsys.com> > > In a corner case scenario, where the height of the assembly TUI is > bigger than the number of instructions in the whole program, GDB > dumps core. The problem roots in this condition check: > > int i = 0; > while (i < content. size() - threshold ...) { > ... content[i] ... > } > > "threshold" is 2 and there are times that "content. size()" is 0. Typo: spurious space in "content. size()", twice. Should be "content.size ()" instead. > This results into an overflow and the loop is entered whereas it > should have been skipped. > > This has been discussed at length in bug 25345: > https://sourceware.org/bugzilla/show_bug.cgi?id=25345 > > As a bonus, a few trailing spaces are also removed. We try to avoid mixing unrelated formatting changes with logical changes. It would be better to push the whitespace fixing as a separate patch. Go ahead and merge that part in as an obvious change. > @@ -349,10 +349,10 @@ bool > tui_disasm_window::addr_is_displayed (CORE_ADDR addr) const > { > bool is_displayed = false; > - int threshold = SCROLL_THRESHOLD; > + int nr_of_lines = (int) content. size() - SCROLL_THRESHOLD; > Someone reading this will have to think through the reason for the int cast, since negative "number of lines" is a bit nonsensical. I suspect that instead doing an early return, like: if (content.size() < SCROLL_THRESHOLD) return false; would end up being clearer? That "while" loop could be a "for" too, no idea why it's not. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed() 2020-01-06 12:18 ` [PATCH v2] GDB: Fix the overflow in addr_is_displayed() Pedro Alves @ 2020-01-06 12:43 ` Shahab Vahedi 2020-01-06 13:03 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Shahab Vahedi @ 2020-01-06 12:43 UTC (permalink / raw) To: Pedro Alves Cc: gdb-patches, Shahab Vahedi, Claudiu Zissulescu, Francois Bedard On Mon, Jan 06, 2020 at 12:17:51PM +0000, Pedro Alves wrote: > On 1/6/20 10:26 AM, Shahab Vahedi wrote: > > From: Shahab Vahedi <shahab@synopsys.com> > > > > In a corner case scenario, where the height of the assembly TUI is > > bigger than the number of instructions in the whole program, GDB > > dumps core. The problem roots in this condition check: > > > > int i = 0; > > while (i < content. size() - threshold ...) { > > ... content[i] ... > > } > > > > "threshold" is 2 and there are times that "content. size()" is 0. > > Typo: spurious space in "content. size()", twice. Should be "content.size ()" > instead. Indeed! Andrew mentioned the same to me. I will fix 'em. > > > This results into an overflow and the loop is entered whereas it > > should have been skipped. > > > > This has been discussed at length in bug 25345: > > https://sourceware.org/bugzilla/show_bug.cgi?id=25345 > > > > As a bonus, a few trailing spaces are also removed. > > We try to avoid mixing unrelated formatting changes with > logical changes. It would be better to push the whitespace > fixing as a separate patch. Go ahead and merge that part > in as an obvious change. I will submit the trailing spaces fix in a separate patch. > > > @@ -349,10 +349,10 @@ bool > > tui_disasm_window::addr_is_displayed (CORE_ADDR addr) const > > { > > bool is_displayed = false; > > - int threshold = SCROLL_THRESHOLD; > > + int nr_of_lines = (int) content. size() - SCROLL_THRESHOLD; > > > > Someone reading this will have to think through the reason for > the int cast, since negative "number of lines" is a bit > nonsensical. I suspect that instead doing an early return, like: > > if (content.size() < SCROLL_THRESHOLD) > return false; > > would end up being clearer? > > That "while" loop could be a "for" too, no idea why it's not. > How does this look? bool is_displayed = false; int threshold = SCROLL_THRESHOLD; - int i = 0; - while (i < content.size () - threshold && !is_displayed) + if (content.size () < threshold) + return is_displayed; + + for (size_t i = 0; i < content.size () - threshold && !is_displayed; ++i) { is_displayed = (content[i].line_or_addr.loa == LOA_ADDRESS && content[i].line_or_addr.u.addr == addr); - i++; } > Thanks, > Pedro Alves > -- Shahab ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed() 2020-01-06 12:43 ` Shahab Vahedi @ 2020-01-06 13:03 ` Pedro Alves 2020-01-06 14:27 ` [PATCH v3] GDB: Fix the overflow in addr/line_is_displayed() Shahab Vahedi 0 siblings, 1 reply; 5+ messages in thread From: Pedro Alves @ 2020-01-06 13:03 UTC (permalink / raw) To: Shahab Vahedi Cc: gdb-patches, Shahab Vahedi, Claudiu Zissulescu, Francois Bedard On 1/6/20 12:43 PM, Shahab Vahedi wrote: > How does this look? > > bool is_displayed = false; > int threshold = SCROLL_THRESHOLD; > > - int i = 0; > - while (i < content.size () - threshold && !is_displayed) > + if (content.size () < threshold) > + return is_displayed; > + I'd write "false" instead of "is_displayed", to remove the indirection. Actually, do we really need the "threshold" variable, btw? Or even, "is_displayed"? Isn't the following equivalent? if (content.size () < SCROLL_THRESHOLD) return false; for (size_t i = 0; i < content.size () - SCROLL_THRESHOLD; i++) { if (content[i].line_or_addr.loa == LOA_ADDRESS && content[i].line_or_addr.u.addr == addr) return true; } return false; Anyway, what you have is fine too. More importantly, doesn't tui_source_window::line_is_displayed have the exact same issue? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3] GDB: Fix the overflow in addr/line_is_displayed() 2020-01-06 13:03 ` Pedro Alves @ 2020-01-06 14:27 ` Shahab Vahedi 2020-01-06 19:55 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Shahab Vahedi @ 2020-01-06 14:27 UTC (permalink / raw) To: gdb-patches Cc: Shahab Vahedi, Pedro Alves, Andrew Burgess, Claudiu Zissulescu, Francois Bedard From: Shahab Vahedi <shahab@synopsys.com> In tui_disasm_window::addr_is_displayed(), there can be situations where "content" is empty. For instance, it can happen when the "content" was not filled in tui_disasm_window::set_contents(), because tui_disassemble() threw an exception. Usually this exception is the result of fetching invalid PC addresses like the ones beyond the end of the program. Having "content.size ()" zero leads to an overflow in this condition check inside tui_disasm_window::addr_is_displayed(): int i = 0; while (i < content.size () - threshold ...) { ... content[i] ... } "threshold" is 2 and there are times that "content.size ()" is 0. This results into an overflow and the loop is entered whereas it should have been skipped. Finally, "content[i]" access leads to a segmentation fault. Same problem applies to tui_source_window::line_is_displayed(). The issue has been discussed at length in bug 25345: https://sourceware.org/bugzilla/show_bug.cgi?id=25345 This commit avoids the segmentation faults with an early check: if (contet.size () < SCROLL_THRESHOLD) return false; Moreover, those functions have been overhauled to a leaner code. gdb/ChangeLog: 2020-01-06 Shahab Vahedi <shahab@synopsys.com> * tui/tui-disasm.c (tui_disasm_window::addr_is_displayed): Avoid overflow by an early check of content vs threshold. * tui/tui-source.c (tui_source_window::line_is_displayed): Likewise. --- gdb/tui/tui-disasm.c | 16 +++++++--------- gdb/tui/tui-source.c | 17 ++++++++--------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c index ebd0ba317f5..98c691f3387 100644 --- a/gdb/tui/tui-disasm.c +++ b/gdb/tui/tui-disasm.c @@ -348,19 +348,17 @@ tui_disasm_window::location_matches_p (struct bp_location *loc, int line_no) bool tui_disasm_window::addr_is_displayed (CORE_ADDR addr) const { - bool is_displayed = false; - int threshold = SCROLL_THRESHOLD; + if (content.size () < SCROLL_THRESHOLD) + return false; - int i = 0; - while (i < content.size () - threshold && !is_displayed) + for (size_t i = 0; i < content.size () - SCROLL_THRESHOLD; ++i) { - is_displayed - = (content[i].line_or_addr.loa == LOA_ADDRESS - && content[i].line_or_addr.u.addr == addr); - i++; + if (content[i].line_or_addr.loa == LOA_ADDRESS + && content[i].line_or_addr.u.addr == addr) + return true; } - return is_displayed; + return false; } void diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c index e028b724d23..1503cd4c636 100644 --- a/gdb/tui/tui-source.c +++ b/gdb/tui/tui-source.c @@ -174,18 +174,17 @@ tui_source_window::location_matches_p (struct bp_location *loc, int line_no) bool tui_source_window::line_is_displayed (int line) const { - bool is_displayed = false; - int threshold = SCROLL_THRESHOLD; - int i = 0; - while (i < content.size () - threshold && !is_displayed) + if (content.size () < SCROLL_THRESHOLD) + return false; + + for (size_t i = 0; i < content.size () - SCROLL_THRESHOLD; ++i) { - is_displayed - = (content[i].line_or_addr.loa == LOA_LINE - && content[i].line_or_addr.u.line_no == line); - i++; + if (content[i].line_or_addr.loa == LOA_LINE + && content[i].line_or_addr.u.line_no == line) + return true; } - return is_displayed; + return false; } void -- 2.24.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] GDB: Fix the overflow in addr/line_is_displayed() 2020-01-06 14:27 ` [PATCH v3] GDB: Fix the overflow in addr/line_is_displayed() Shahab Vahedi @ 2020-01-06 19:55 ` Pedro Alves 0 siblings, 0 replies; 5+ messages in thread From: Pedro Alves @ 2020-01-06 19:55 UTC (permalink / raw) To: Shahab Vahedi, gdb-patches Cc: Shahab Vahedi, Andrew Burgess, Claudiu Zissulescu, Francois Bedard Hi, I merged this patch without waiting for copyright clearing, since the amount of new copyrightable code is a few lines only IMO (the threshold check), and also the current form of the patch was largely suggested by me in a previous email. For further fixes, I think it's better to wait until copyright issues are cleared though. > This commit avoids the segmentation faults with an early check: > > if (contet.size () < SCROLL_THRESHOLD) > return false; I fixed the "contet" typo before merging. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-06 19:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200106102649.15710-1-shahab.vahedi@gmail.com>
2020-01-06 12:18 ` [PATCH v2] GDB: Fix the overflow in addr_is_displayed() Pedro Alves
2020-01-06 12:43 ` Shahab Vahedi
2020-01-06 13:03 ` Pedro Alves
2020-01-06 14:27 ` [PATCH v3] GDB: Fix the overflow in addr/line_is_displayed() Shahab Vahedi
2020-01-06 19:55 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox