[2/2] Make 'show width/height' display "unlimited" when capped, for readline (Re: [PATCH] Prevent overflow in rl_set_screen_size)

Message ID 300b07b2-3803-a573-33a2-049b630a6164@redhat.com
State New
Headers show
Series
  • Untitled series #11504
Related show

Commit Message

Pedro Alves Feb. 20, 2019, 5:37 p.m.
On 02/20/2019 05:22 PM, Pedro Alves wrote:
> 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?


And this as a follow up?

From f9124a178e0aeacfa63f18aa742f6b7c8762051b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 20 Feb 2019 17:12:03 +0000
Subject: [PATCH 2/2] Make 'show width/height' display "unlimited" when capped
 for readline

When we cap the height/width sizes before passing to readline, tweak
the corresponding command variable to show "unlimited":

  (gdb) set height 0x8000
  (gdb) show height
  Number of lines gdb thinks are in a page is unlimited.

Instead of the current output:
  (gdb) set height 0x8000
  (gdb) show height
  Number of lines gdb thinks are in a page is 32768.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* utils.c (set_screen_size): When we cap the height/width sizes,
	tweak the corresponding command variable to show "unlimited":

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/page.exp: Add tests for "set/show width/height" with
	"infinite" values.
---
 gdb/testsuite/gdb.base/page.exp | 24 ++++++++++++++++++++++++
 gdb/utils.c                     | 10 ++++++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.14.4

Comments

Tom Tromey Feb. 20, 2019, 9:04 p.m. | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> So how about this version instead?

Pedro> And this as a follow up?

These both look good to me.
Thanks for following up on this.

Tom
Kevin Buettner Feb. 20, 2019, 11:01 p.m. | #2
On Wed, 20 Feb 2019 14:04:28 -0700
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:  

> 

> Pedro> So how about this version instead?  

> 

> Pedro> And this as a follow up?  

> 

> These both look good to me.

> Thanks for following up on this.


LGTM too.

Kevin
Pedro Alves Feb. 27, 2019, 6:50 p.m. | #3
On 02/20/2019 11:01 PM, Kevin Buettner wrote:
> On Wed, 20 Feb 2019 14:04:28 -0700

> Tom Tromey <tom@tromey.com> wrote:

> 

>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:  

>>

>> Pedro> So how about this version instead?  

>>

>> Pedro> And this as a follow up?  

>>

>> These both look good to me.

>> Thanks for following up on this.

> 

> LGTM too.

Thanks.  I've checked these in now.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp
index 10ebf0d43b..8f1698c26e 100644
--- a/gdb/testsuite/gdb.base/page.exp
+++ b/gdb/testsuite/gdb.base/page.exp
@@ -94,6 +94,30 @@  gdb_expect_list "paged count for interrupt" \
 
 gdb_test "q" "Quit" "quit while paging"
 
+# Check that width/height of sqrt(INT_MAX) is treated as unlimited, as
+# well as "0" and explicit "unlimited".
+foreach_with_prefix size {"0" "0x80000000" "unlimited"} {
+
+    # Alternate between "non-unlimited" values and "unlimited" values,
+    # to make sure we're not seeing stale internal state.
+
+    gdb_test "set width 200"
+    gdb_test "show width" \
+	"Number of characters gdb thinks are in a line is 200\\."
+
+    gdb_test "set height 200"
+    gdb_test "show height" \
+	"Number of lines gdb thinks are in a page is 200\\."
+
+    gdb_test "set width $size"
+    gdb_test "show width unlimited" \
+	"Number of characters gdb thinks are in a line is unlimited\\."
+
+    gdb_test "set height $size"
+    gdb_test "show height unlimited" \
+	"Number of lines gdb thinks are in a page is unlimited\\."
+}
+
 gdb_exit
 return 0
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 069da23542..60af31f2e4 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1394,10 +1394,16 @@  set_screen_size (void)
   const int sqrt_int_max = INT_MAX >> (sizeof (int) * 8 / 2);
 
   if (rows <= 0 || rows > sqrt_int_max)
-    rows = sqrt_int_max;
+    {
+      rows = sqrt_int_max;
+      lines_per_page = UINT_MAX;
+    }
 
   if (cols <= 0 || cols > sqrt_int_max)
-    cols = sqrt_int_max;
+    {
+      cols = sqrt_int_max;
+      chars_per_line = UINT_MAX;
+    }
 
   /* Update Readline's idea of the terminal size.  */
   rl_set_screen_size (rows, cols);