Re: [PATCH] Prevent overflow in rl_set_screen_size

[Saagar Jha <saagar@saagarjha.com>]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

Here’s a new patch. I can try to see what readline is willing to do regarding documenting this limitation, but that might take a while because I’d have to figure out where to send contributions and the process to getting them merged in (I *think* I’m supposed to send an email to  bug-readline@gnu.org?) Hopefully this patch is useful on its own until readline is fixed.

Regards,
Saagar Jha


[-- Attachment #2: Prevent-overflow-in-rl_set_screen_size.patch --]
[-- Type: application/octet-stream, Size: 1622 bytes --]

From 7955608ce06dbd8944292b2318b90863d3a82ae1 Mon Sep 17 00:00:00 2001
From: Saagar Jha <saagar@saagarjha.com>
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:
2019-02-15  Saagar Jha  <saagar@saagarjha.com>

	* utils.c (set_screen_size): Reduce "infinite" rows and columns
	before calling rl_set_screen_size.
---
 gdb/utils.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 6fb5736abb..abca7337ab 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1376,11 +1376,13 @@ set_screen_size (void)
   int rows = lines_per_page;
   int cols = chars_per_line;
 
+  /* Use approximately sqrt(INT_MAX) instead of INT_MAX so that we don't
+     overflow in rl_set_screen_size, which multiplies rows and columns. */
   if (rows <= 0)
-    rows = INT_MAX;
+    rows = INT_MAX >> (sizeof( int ) * 8 / 2);
 
   if (cols <= 0)
-    cols = INT_MAX;
+    cols = INT_MAX >> (sizeof( int ) * 8 / 2);
 
   /* Update Readline's idea of the terminal size.  */
   rl_set_screen_size (rows, cols);
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 2424 bytes --]


> On Feb 15, 2019, at 01:39, Pedro Alves <palves@redhat.com> wrote:
> 
> On 02/15/2019 01:52 AM, Kevin Buettner wrote:
>> On Fri, 26 Oct 2018 21:56:50 -0700
>> Saagar Jha <saagar@saagarjha.com> wrote:
>> 
>>> 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.
>> 
>> This seems like a reasonable approach to me.  (I couldn't think of a
>> better way to do it.)
> 
> It might be reasonable to have this as workaround, but pedantically,
> shouldn't this be fixed in readline?  The function's
> documentation doesn't say anything about upper limits:
> 
> "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."
> 
> so if the function takes int parameters without specifying an upper bound, it
> seems like a readline bug to me to not consider large numbers.
> 
> A couple comments on formatting below.
> 
>>> diff --git a/gdb/utils.c b/gdb/utils.c
>>> index 8d4a744e71..56257c35cf 100644
>>> --- a/gdb/utils.c
>>> +++ b/gdb/utils.c
>>> @@ -1377,11 +1377,13 @@ set_screen_size (void)
>>>   int rows = lines_per_page;
>>>   int cols = chars_per_line;
>>> 
>>> +  // Use approximately sqrt(INT_MAX) instead of INT_MAX so that we don't
>>> +  // overflow in rl_set_screen_size, which multiplies rows and columns
> 
> Please use /**/ for comments, and end the sentence with a period.
> 
>>>   if (rows <= 0)
>>> -    rows = INT_MAX;
>>> +    rows = INT_MAX >> (sizeof(int) * 8 / 2);
> 
> Space before parens in "sizeof(int)".
> 
>>> 
>>>   if (cols <= 0)
>>> -    cols = INT_MAX;
>>> +    cols = INT_MAX >> (sizeof(int) * 8 / 2);
> 
> Ditto.
> 
>>> 
>>>   /* Update Readline's idea of the terminal size.  */
>>>   rl_set_screen_size (rows, cols);
>>> -- 
>>> 2.19.1
>>> 
>>> 
> 
> 
> Thanks,
> Pedro Alves