Re: [PATCH] Prevent overflow in rl_set_screen_size

[Pedro Alves <palves at redhat dot com>]

On 02/20/2019 03:54 PM, Pedro Alves wrote:
> On 02/15/2019 08:19 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> 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 <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:
yyyy-mm-dd  Saagar Jha  <saagar@saagarjha.com>
	    Pedro Alves  <palves@redhat.com>

	* 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