From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16529 invoked by alias); 4 Jun 2012 21:54:43 -0000 Received: (qmail 16519 invoked by uid 22791); 4 Jun 2012 21:54:42 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 04 Jun 2012 21:54:25 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q54LsOPi007579 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 4 Jun 2012 17:54:25 -0400 Received: from host2.jankratochvil.net (ovpn-116-47.ams2.redhat.com [10.36.116.47]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q54LsKUQ008597 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 4 Jun 2012 17:54:23 -0400 Date: Mon, 04 Jun 2012 21:54:00 -0000 From: Jan Kratochvil To: Siddhesh Poyarekar Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH] Memory reads and writes should have size_t length Message-ID: <20120604215419.GA27980@host2.jankratochvil.net> References: <20120531125320.65ad1f8f@spoyarek> <20120601174809.GA21938@host2.jankratochvil.net> <20120602012958.4a6d9a7c@spoyarek> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120602012958.4a6d9a7c@spoyarek> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 X-SW-Source: 2012-06/txt/msg00104.txt.bz2 On Fri, 01 Jun 2012 21:59:58 +0200, Siddhesh Poyarekar wrote: > On Fri, 1 Jun 2012 19:48:09 +0200, Jan wrote: > > This patch goes again more far than what is needed, couldn't this be > > ssize_t? Making it unsigned could be some other cleanup. > > > > I took the liberty of changing signs here because this patch in itself > is small enough (and independent) But read_memory has 120 callers to check, other functions have hundreds of other callers to check. I have found at least the patch below to catch some of these cases but it is far from complete. Currently negative values were harmless on 32-bit hosts but now they will overwrite GDB address space: #include void target_read(long long x) { printf("%llx, 0 and if it does cause a regression, it > should be pretty easy to isolate even with a simple bisect, I do not think we need to cause regressions here, ssize_t has no practical disadvantages compared to size_t, just it is not so clean/nice. > This patch can be tested independently, so I figured this was OK. What > do you think? Unless someone else is going to protect all the hundreds/thousands of callers I do not think it is worth it and ssize_t is good enough. Thanks, Jan diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c index 0e441fb..1636216 100644 --- a/gdb/ada-tasks.c +++ b/gdb/ada-tasks.c @@ -427,6 +427,8 @@ read_fat_string_value (char *dest, struct value *val, int max_len) The lower bound is always 1, so we only need to read the upper bound. */ bounds_val = value_ind (value_field (val, bounds_fieldno)); len = value_as_long (value_field (bounds_val, upper_bound_fieldno)); + if (len < 0) + error (_("Invalid task format. Aborting.")); /* Make sure that we do not read more than max_len characters... */ if (len > max_len) diff --git a/gdb/valops.c b/gdb/valops.c index feb47f5..ca4ee26 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1282,7 +1282,7 @@ value_assign (struct value *toval, struct value *fromval) && ((LONGEST) changed_addr % TYPE_LENGTH (type)) == 0) changed_len = TYPE_LENGTH (type); - if (changed_len > (int) sizeof (LONGEST)) + if (changed_len > (int) sizeof (LONGEST) || changed_len < 0) error (_("Can't handle bitfields which " "don't fit in a %d bit word."), (int) sizeof (LONGEST) * HOST_CHAR_BIT);