From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76572 invoked by alias); 6 Jan 2020 12:43:59 -0000 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 Received: (qmail 76562 invoked by uid 89); 6 Jan 2020 12:43:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,FSL_HELO_FAKE,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*RU:gmail.com, HX-Spam-Relays-External:gmail.com, H*r:gmail.com X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 Jan 2020 12:43:57 +0000 Received: by mail-wm1-f68.google.com with SMTP id p9so14810742wmc.2 for ; Mon, 06 Jan 2020 04:43:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-description:content-disposition:in-reply-to; bh=cMBu9H9l8F5F8xJkZ7XXMIiGr9vJFzx4+/xnEz6yB5k=; b=o51oCjGETsq9lU6okB0XklYt41E5yEd6ljQ8PNdsn55vOqPkUk5hy3BR+MSddoZ7gL 4tqoIyyvCjGVmBwEJgJABx/WOJQxHXe9ludRK8z6LqCBmCOAow95xAdz+AOAZnAbSmp+ RljQ82OBcEEhNjsahtNioI2Q5raV2jY+keVF+hlQIRRcy4wv4HruVHV9/AryfvfJQ6UG VmVAK4IpA4fX1epOZtzCapmFO+w57icwRv5WcPKLtd1RjnmQ8p50g6iIYSfMl0p9qSfb /MwmwN7G2lC0QIt783EFGqBwpzrFiHKgLxdCcweaxb/iP/ojHvekKUXvAMrKf/qZHhEn kZ+A== Return-Path: Received: from gmail.com ([2a03:1b20:3:f011::6d]) by smtp.gmail.com with ESMTPSA id f1sm72597279wru.6.2020.01.06.04.43.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jan 2020 04:43:55 -0800 (PST) Date: Mon, 06 Jan 2020 12:43:00 -0000 From: Shahab Vahedi To: Pedro Alves Cc: gdb-patches@sourceware.org, Shahab Vahedi , Claudiu Zissulescu , Francois Bedard Subject: Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed() Message-ID: <20200106124353.GB1101@gmail.com> References: <20200106102649.15710-1-shahab.vahedi@gmail.com> <11035b53-bb43-740a-de38-6283062cdc6d@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Description: all_response Content-Disposition: inline In-Reply-To: <11035b53-bb43-740a-de38-6283062cdc6d@redhat.com> X-SW-Source: 2020-01/txt/msg00106.txt.bz2 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 > > > > 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