From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18545 invoked by alias); 26 Mar 2013 10:23:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18506 invoked by uid 89); 26 Mar 2013 10:23:12 -0000 X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 26 Mar 2013 10:23:09 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1UKR22-00054O-NW from Yao_Qi@mentor.com ; Tue, 26 Mar 2013 03:23:06 -0700 Received: from SVR-ORW-FEM-04.mgc.mentorg.com ([147.34.97.41]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Tue, 26 Mar 2013 03:23:06 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.1.289.1; Tue, 26 Mar 2013 03:23:05 -0700 Message-ID: <515176BB.4000702@codesourcery.com> Date: Tue, 26 Mar 2013 16:48:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Pedro Alves CC: , Subject: Re: Change "set history size" back to signed (Re: [committed]: [PATCH 1/3] var_integer -> var_uinteger) References: <7A6A55B4-0293-4AD6-AB1F-B3169F8ADCC1@cs.umd.edu> <1344871663-915-1-git-send-email-yao@codesourcery.com> <1344871663-915-2-git-send-email-yao@codesourcery.com> <20534.29766.629459.204573@ruffy2.mtv.corp.google.com> <50372591.7080404@codesourcery.com> <20535.46196.619465.922388@ruffy2.mtv.corp.google.com> <503B476F.50805@codesourcery.com> <20539.61738.119545.634687@ruffy2.mtv.corp.google.com> <503CD0F7.20906@codesourcery.com> <5150A09D.3090202@redhat.com> In-Reply-To: <5150A09D.3090202@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-SW-Source: 2013-03/txt/msg00964.txt.bz2 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 * 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