Re: [PATCH] Prevent overflow in rl_set_screen_size

[Saagar Jha <saagar at saagarjha dot com>]

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: Prevent-overflow-in-rl_set_screen_size.patch
Description: Binary data

> 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