Re: Change "set history size" back to signed (Re: [committed]: [PATCH 1/3] var_integer -> var_uinteger)

[Yao Qi <yao at codesourcery dot com>]

On 03/26/2013 03:08 AM, Pedro Alves wrote:
> I'd like to revert "set history size" back to signed.
> 
> http://sourceware.org/ml/gdb-patches/2012-08/msg00832.html
> 
> All the variables associated with the command are int,
> and they need to be, because that's how the readline interfaces
> are defined.
> 

Pedro,
I understand that your point of choosing signed command is readline
interface is signed.  However, my point of choosing unsigned command
is the characteristic of command "set history size" is unsigned.  I
don't know why readline interface is signed, but I don't want this
affect or restrict the design of the command.

> As it stands, this introduced signed/unsigned comparisons
> and undefined overflows, like in:
> 
> void
> show_commands (char *args, int from_tty)
> {
>    /* Index for history commands.  Relative to history_base.  */
>    int offset;
> ...
>    /* Print out some of the commands from the command history.  */
>    /* First determine the length of the history list.  */
>    hist_len = history_size;
>    for (offset = 0; offset < history_size; offset++)
>                     ^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^
>      {
>        if (!history_get (history_base + offset))
> ...
>      }
> 
> With history_size set to UINT_MAX, with a (very) large history,
> that will overflow the signed "offset".  Making "offset" itself
> unsigned is useless, as then we'd overflow on the call to
> history_get.

The potential overflow needs to be fixed.  I prefer to change "offset"
to unsigned, and check the range when passing "history_base + offset"
to history_get.  Change everything to signed still has the risk of
overflow in "history_get (history_base + offset)".

> 
> The fact that all the code that uses these interfaces and
> variables is "signed", and that "history_size" ends up trimmed
> to INT_MAX anyway:
> 
> /* Called by do_setshow_command.  */
> static void
> set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
> {
>    /* The type of parameter in stifle_history is int, so values from INT_MAX up
>       mean 'unlimited'.  */
>    if (history_size >= INT_MAX)
>      {
>        /* Ensure that 'show history size' prints 'unlimited'.  */
>        history_size = UINT_MAX;
>        unstifle_history ();
>      }
>    else
>      stifle_history (history_size);
> }
> 
> very much reads to me that making this "unsigned" is not
> justified.
> 
> This patch changes the command back to var_integer.  It's
> not a reversion -- I'm doing things differently from what
> was done before.  Namely, if a negative value is specified,
> the user sees the exact same error the uinteger command throws.
> Also, in that case, the command reverts back to the previous
> setting, like current code does implicitly, but unlike the original,
> that would change the setting to the default on range error.
> IOW, there's no user visible change compared to the current code.

Your patch changes the command back to signed, and at the same time,
keep the command behaves like unsigned.  This makes code a little bit
hard, IMO.  Isn't better to keep the command unsigned (from the
user's perspective) and take care of potential signed overflow
(to fit the internal readline interface)?  How about the patch
below?
 
-- 
Yao (éå)

gdb:

2013-03-26  Yao Qi  <yao@codesourcery.com>

	* top.c (show_commands): Change local var 'offset' to
	'unsigned int'.  Check the overflow of 'history_base + offset'
	before pass it to history_get.
---
 gdb/top.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 7905b51..1cedfe9 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1373,7 +1373,7 @@ void
 show_commands (char *args, int from_tty)
 {
   /* Index for history commands.  Relative to history_base.  */
-  int offset;
+  unsigned int offset;
 
   /* Number of the history entry which we are planning to display next.
      Relative to history_base.  */
@@ -1388,7 +1388,10 @@ show_commands (char *args, int from_tty)
   hist_len = history_size;
   for (offset = 0; offset < history_size; offset++)
     {
-      if (!history_get (history_base + offset))
+      /* Avoid overflow when passing 'history_base + offset' to
+	 history_get.  */
+      if ((offset + history_base > INT_MAX)
+	  || !history_get (history_base + offset))
 	{
 	  hist_len = offset;
 	  break;
-- 
1.7.7.6