From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67684 invoked by alias); 6 Jan 2020 00:30:56 -0000 Mailing-List: contact gdb-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org Received: (qmail 67596 invoked by uid 89); 6 Jan 2020 00:30:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=corner, xxx, H*RU:209.85.128.68, HX-Spam-Relays-External:209.85.128.68 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 00:30:45 +0000 Received: by mail-wm1-f68.google.com with SMTP id q9so13424965wmj.5 for ; Sun, 05 Jan 2020 16:30:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=pmUOLNdK+lqveDsrCQZJ51x4wdQffHZbbGvUD/BMB5E=; b=COAwrWit+Jn5v+kdHDZNBXuAzVhym44Qr5ucUAkDfN9FIJMe0CrWB/bzdtZDU0HUii my7EogkRZPInxBBxbAiI+GnNGZYNs7+gK2/Nz1dyCG+4gx1eVUgK4sJtlcaRKmRdIyNl uh31cLkeL48dFTwN4lsjeIaQXoeQBZZ3JI6ZbKa1Q0Hyi8vKLgUdFkdJUqvLG30R5IFF auxq3wbnA1qGAm4/IiM6wf2MPgTV/9ndAOvHpY+0jA9ws26ZZu5YhM37+oEXI40FdXIp tGiK2Y78GakLBrsoMF4mYPMLGmYxxJy5mV62Bu8wATBFYS808dt0qPh0vIC6fYWQXOVV 8uvQ== Return-Path: Received: from localhost (host86-186-80-236.range86-186.btcentralplus.com. [86.186.80.236]) by smtp.gmail.com with ESMTPSA id i5sm20943279wml.31.2020.01.05.16.30.42 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 05 Jan 2020 16:30:42 -0800 (PST) Date: Mon, 06 Jan 2020 00:30:00 -0000 From: Andrew Burgess To: Shahab Vahedi Cc: gdb-patches@sourceware.org, gdb@sourceware.org, Shahab Vahedi , Claudiu Zissulescu , Francois Bedard Subject: Re: [PATCH] GDB: Fix the overflow in addr_is_displayed() Message-ID: <20200106003041.GS3865@embecosm.com> References: <20200104114312.165656-1-shahab.vahedi@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200104114312.165656-1-shahab.vahedi@gmail.com> X-Fortune: With listening comes wisdom, with speaking repentance. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00004.txt.bz2 * Shahab Vahedi [2020-01-04 12:43:12 +0100]: > 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. > This results into an overflow and the loop is entered whereas it > should have been skipped. I didn't quite understand the problem description. I can see how 'content.size() - threshold' would overflow when the size is 0 or 1, but I don't understand the part about the tui height being bigger than the number of instructions. I tried to reproduce the failure on native x86-64 but failed. I can get the initial stage reproduced, where the asm window is empty and I see the error about "Cannot access memory at address ....", but then when I attach to a remote the asm window fills in fine. I guess this is because on Linux the page the code is on is readable, and under QEMU only the specific program instructions are readable maybe? Anyway, the change looks reasonable, though I had one comment, inline below... > > This has been discussed at length in bug 25345: > https://sourceware.org/bugzilla/show_bug.cgi?id=25345 > > gdb/ChangeLog: > 2020-01-04 Shahab Vahedi > * tui/tui-disasm.c (tui_disasm_window::addr_is_displayed): > Treat "content.size()" as "int" to avoid overflow. > --- > gdb/tui/tui-disasm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c > index c72b50730b0..a0921eb09d1 100644 > --- a/gdb/tui/tui-disasm.c > +++ b/gdb/tui/tui-disasm.c > @@ -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()) - int(SCROLL_THRESHOLD); I had a look through our code and I couldn't find any examples of us using 'int (xxx)' syntax to cast to int, this is probably a result of our C heritage, but we should probably try to be consistent I think. I'd suggest maybe this become: int nr_of_lines = (int) content.size () - SCROLL_THRESHOLD; Thanks, Andrew > > int i = 0; > - while (i < content.size () - threshold && !is_displayed) > + while (i < nr_of_lines && !is_displayed) > { > is_displayed > = (content[i].line_or_addr.loa == LOA_ADDRESS > -- > 2.24.1 >