From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91042 invoked by alias); 20 Feb 2019 17:22:39 -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 91028 invoked by uid 89); 20 Feb 2019 17:22:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=screen, cap 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; Wed, 20 Feb 2019 17:22:37 +0000 Received: by mail-wm1-f68.google.com with SMTP id x10so7143699wmg.2 for ; Wed, 20 Feb 2019 09:22:36 -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 c1sm16878838wrw.7.2019.02.20.09.22.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Feb 2019 09:22:34 -0800 (PST) Subject: 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: Date: Wed, 20 Feb 2019 17:22:00 -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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-02/txt/msg00332.txt.bz2 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? I've also extended the comment based on my previous email. >From ce69f10a95fea289676bfb5db58b096546befe4f Mon Sep 17 00:00:00 2001 From: Saagar Jha Date: Tue, 22 May 2018 04:08:40 -0700 Subject: [PATCH] Prevent overflow in rl_set_screen_size GDB calls rl_set_screen_size in readline with the current screen size, measured in rows and columns. To represent "infinite" sizes, GDB passes in INT_MAX; however, since rl_set_screen_size internally multiplies the number of rows and columns, this causes a signed integer overflow. To prevent this we can instead pass in the approximate square root of INT_MAX (which is still reasonably large), so that even when the number of rows and columns is "infinite" we don't overflow. gdb/ChangeLog: yyyy-mm-dd Saagar Jha Pedro Alves * utils.c (set_screen_size): Reduce "infinite" rows and columns before calling rl_set_screen_size. --- gdb/utils.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/gdb/utils.c b/gdb/utils.c index ec2619642a..069da23542 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1380,11 +1380,24 @@ set_screen_size (void) int rows = lines_per_page; int cols = chars_per_line; - if (rows <= 0) - rows = INT_MAX; + /* If we get 0 or negative ROWS or COLS, treat as "infinite" size. + A negative number can be seen here with the "set width/height" + commands and either: - if (cols <= 0) - cols = INT_MAX; + - the user specified "unlimited", which maps to UINT_MAX, or + - the user spedified some number between INT_MAX and UINT_MAX. + + Cap "infinity" to approximately sqrt(INT_MAX) so that we don't + overflow in rl_set_screen_size, which multiplies rows and columns + to compute the number of characters on the screen. */ + + const int sqrt_int_max = INT_MAX >> (sizeof (int) * 8 / 2); + + if (rows <= 0 || rows > sqrt_int_max) + rows = sqrt_int_max; + + if (cols <= 0 || cols > sqrt_int_max) + cols = sqrt_int_max; /* Update Readline's idea of the terminal size. */ rl_set_screen_size (rows, cols); -- 2.14.4