Re: [PATCH] Prevent overflow in rl_set_screen_size

[Pedro Alves <palves@redhat.com>]

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