From patchwork Wed Feb 20 17:37:22 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31537 Received: (qmail 56018 invoked by alias); 20 Feb 2019 17:37:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55475 invoked by uid 89); 20 Feb 2019 17:37:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Feb 2019 17:37:25 +0000 Received: by mail-wr1-f65.google.com with SMTP id w2so12401233wrt.11 for ; Wed, 20 Feb 2019 09:37:25 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:75e6:857f:3506:a1f4? ([2001:8a0:f913:f700:75e6:857f:3506:a1f4]) by smtp.gmail.com with ESMTPSA id a62sm5093601wmf.47.2019.02.20.09.37.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Feb 2019 09:37:23 -0800 (PST) Subject: [PATCH 2/2] Make 'show width/height' display "unlimited" when capped, for readline (Re: [PATCH] Prevent overflow in rl_set_screen_size) To: Tom Tromey References: <20190214185254.15369a0a@f29-4.lan> <9e42397a-e3a5-0296-d239-70f4c7c0d215@redhat.com> <87wom0yfcb.fsf@tromey.com> Cc: Kevin Buettner , gdb-patches@sourceware.org, Saagar Jha From: Pedro Alves Message-ID: <300b07b2-3803-a573-33a2-049b630a6164@redhat.com> Date: Wed, 20 Feb 2019 17:37:22 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: On 02/20/2019 05:22 PM, Pedro Alves wrote: > On 02/20/2019 03:54 PM, Pedro Alves wrote: >> On 02/15/2019 08:19 PM, Tom Tromey wrote: >>>>>>>> "Pedro" == Pedro Alves writes: >>> >>> Pedro> so if the function takes int parameters without specifying an upper bound, it >>> Pedro> seems like a readline bug to me to not consider large numbers. >>> >>> True, though it doesn't hurt to also check in gdb. >> >> Yeah, that's what I meant to imply with >> "It might be reasonable to have this as workaround" >> Maybe not the best choice of words. >> >>> >>> What's funny is that readline *does* check for negative values: >>> >>> if (rows > 0) >>> _rl_screenheight = rows; >>> .. etc .. >> >> Yeah, it's implementing what is documented: >> >> "Function: void rl_set_screen_size (int rows, int cols) >> Set Readline's idea of the terminal size to rows rows and cols columns. >> If either rows or columns is less than or equal to 0, Readline's idea >> ^^^^^^^^^^^^^^^^^^^^^^^ >> of that terminal dimension is unchanged." >> >> Note the "less than or equal to 0". I would assume that that >> comes from a "it's obviously bogus to have negative sizes, so we'll >> just ignore them" perspective. >> >>> >>> So gdb's approach: >>> >>> if (rows <= 0) >>> rows = INT_MAX; >>> >>> ... actively works around the existing checks in readline. >> >> I'd call it more like mapping different ranges/APIs. gdb >> uses "0" or UINT_MAX for unlimited: >> >> (gdb) help set height >> Set number of lines in a page for GDB output pagination. >> This affects the number of lines after which GDB will pause >> its output and ask you whether to continue. >> Setting this to "unlimited" or zero causes GDB never pause during output. >> >> While negative numbers don't work on the command line: >> >> (gdb) set height -1 >> integer -1 out of range >> >> you end up with negative rows/cols in that quoted code if you >> do "set height/width unlimited", because lines_per_page/chars_per_line >> are unsigned integers and "unlimited" sets them to UINT_MAX. And >> also, if you do >> (gdb) set height 'unsigned number between INT_MAX and UINT_MAX' >> like: >> (gdb) set height 0x80000000 >> >> then that code: >> >> if (rows <= 0) >> rows = INT_MAX; >> >> maps the value to INT_MAX, which is basically the same thing in >> practice -- a huge number is practically the same as "unlimited" here. > > Which makes me think that to be 100% correct wrt to avoiding overflow in > readline, we should also treat numbers >= sqrt(INT_MAX) as "infinite". > Because otherwise, with > > (gdb) set height 0x7ffff > (gdb) set width 0x7ffff > > readline overflows too, even with Saagar's current patch, obviously > because 0x7ffff x 0x7ffff overflows int: > > (gdb) p 0x7ffff * 0x7ffff > $1 = -1048575 > > So how about this version instead? And this as a follow up? From f9124a178e0aeacfa63f18aa742f6b7c8762051b Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 20 Feb 2019 17:12:03 +0000 Subject: [PATCH 2/2] Make 'show width/height' display "unlimited" when capped for readline When we cap the height/width sizes before passing to readline, tweak the corresponding command variable to show "unlimited": (gdb) set height 0x8000 (gdb) show height Number of lines gdb thinks are in a page is unlimited. Instead of the current output: (gdb) set height 0x8000 (gdb) show height Number of lines gdb thinks are in a page is 32768. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * utils.c (set_screen_size): When we cap the height/width sizes, tweak the corresponding command variable to show "unlimited": gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves * gdb.base/page.exp: Add tests for "set/show width/height" with "infinite" values. --- gdb/testsuite/gdb.base/page.exp | 24 ++++++++++++++++++++++++ gdb/utils.c | 10 ++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp index 10ebf0d43b..8f1698c26e 100644 --- a/gdb/testsuite/gdb.base/page.exp +++ b/gdb/testsuite/gdb.base/page.exp @@ -94,6 +94,30 @@ gdb_expect_list "paged count for interrupt" \ gdb_test "q" "Quit" "quit while paging" +# Check that width/height of sqrt(INT_MAX) is treated as unlimited, as +# well as "0" and explicit "unlimited". +foreach_with_prefix size {"0" "0x80000000" "unlimited"} { + + # Alternate between "non-unlimited" values and "unlimited" values, + # to make sure we're not seeing stale internal state. + + gdb_test "set width 200" + gdb_test "show width" \ + "Number of characters gdb thinks are in a line is 200\\." + + gdb_test "set height 200" + gdb_test "show height" \ + "Number of lines gdb thinks are in a page is 200\\." + + gdb_test "set width $size" + gdb_test "show width unlimited" \ + "Number of characters gdb thinks are in a line is unlimited\\." + + gdb_test "set height $size" + gdb_test "show height unlimited" \ + "Number of lines gdb thinks are in a page is unlimited\\." +} + gdb_exit return 0 diff --git a/gdb/utils.c b/gdb/utils.c index 069da23542..60af31f2e4 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1394,10 +1394,16 @@ set_screen_size (void) const int sqrt_int_max = INT_MAX >> (sizeof (int) * 8 / 2); if (rows <= 0 || rows > sqrt_int_max) - rows = sqrt_int_max; + { + rows = sqrt_int_max; + lines_per_page = UINT_MAX; + } if (cols <= 0 || cols > sqrt_int_max) - cols = sqrt_int_max; + { + cols = sqrt_int_max; + chars_per_line = UINT_MAX; + } /* Update Readline's idea of the terminal size. */ rl_set_screen_size (rows, cols);