From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38363 invoked by alias); 6 Jan 2020 12:18:06 -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 38323 invoked by uid 89); 6 Jan 2020 12:18:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 Jan 2020 12:18:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578313081; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0PaKvklfKDAf80qOQLw43jYHzM+qzyX6O6EzIvLR9wA=; b=VWkCfd6F0uo6xov4c1OGWKbL8COYWf+t/BdTNGTCZZ7NOxGxWpKX2kdbS6CfDc/hSqRHym mW5yuARoul/VMAWfE5ceJlXTDeFkRztjq948QDbW6YaJEo0dBArWGPM2ZKI0Fs6Sy8NuPQ 0+ZmGvINpdCdBYoEqfTSlDNCGvuruBk= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-47-utJhesOsP3qJPp8rZrQpVg-1; Mon, 06 Jan 2020 07:17:54 -0500 Received: by mail-wr1-f70.google.com with SMTP id t3so26257365wrm.23 for ; Mon, 06 Jan 2020 04:17:54 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id o194sm23180964wme.45.2020.01.06.04.17.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jan 2020 04:17:52 -0800 (PST) Subject: Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed() To: Shahab Vahedi , gdb-patches@sourceware.org References: <20200106102649.15710-1-shahab.vahedi@gmail.com> Cc: Shahab Vahedi , Claudiu Zissulescu , Francois Bedard From: Pedro Alves Message-ID: <11035b53-bb43-740a-de38-6283062cdc6d@redhat.com> Date: Mon, 06 Jan 2020 12:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200106102649.15710-1-shahab.vahedi@gmail.com> X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-01/txt/msg00105.txt.bz2 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. > 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