From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39153 invoked by alias); 1 Nov 2018 15:35:23 -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 39139 invoked by uid 89); 1 Nov 2018 15:35:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=gdb_byte, our X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Nov 2018 15:35:21 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8AA495602E; Thu, 1 Nov 2018 11:35:19 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id mGbCYhzUcPDi; Thu, 1 Nov 2018 11:35:19 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 463B45602C; Thu, 1 Nov 2018 11:35:19 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 81BCD83AF9; Thu, 1 Nov 2018 08:35:17 -0700 (PDT) Date: Thu, 01 Nov 2018 15:35:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix buffer overflow in ada-lang.c:move_bits Message-ID: <20181101153517.GA2705@adacore.com> References: <20181024162037.21024-1-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181024162037.21024-1-tom@tromey.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2018-11/txt/msg00004.txt.bz2 Hi Tom, > -fsanitize=address showed that ada-lang.c:move_bits can run off the > end of the source buffer. I believe this patch fixes the problem, by > arranging not to read from the source buffer once there are sufficient > bits in the accumulator. > > gdb/ChangeLog > 2018-10-23 Tom Tromey > > * ada-lang.c (move_bits): Don't run off the end of the source > buffer. Thanks for the patch! This is a part of the code that always forces me to think twice (or ten times), each time I try to touch it. I should really start adding comments to this code that detail what we are trying to do as we do it. I tested your change through our testsuite on the various baremetal targets we have, and noticed that it causes regressions on ppc and arm targets. It's hopefully something small, but just being back from a holiday, I'm a bit tied up at work; I'll put that issue on my TODO list to look at further. > --- > gdb/ChangeLog | 5 +++++ > gdb/ada-lang.c | 18 ++++++++++++------ > 2 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index 1462271a71..7288d65df6 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -2682,9 +2682,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source, > { > int unused_right; > > - accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source; > - accum_bits += HOST_CHAR_BIT; > - source += 1; > + if (n > accum_bits) > + { > + accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source; > + accum_bits += HOST_CHAR_BIT; > + source += 1; > + } > chunk_size = HOST_CHAR_BIT - targ_offset; > if (chunk_size > n) > chunk_size = n; > @@ -2707,9 +2710,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source, > > while (n > 0) > { > - accum = accum + ((unsigned char) *source << accum_bits); > - accum_bits += HOST_CHAR_BIT; > - source += 1; > + if (n > accum_bits) > + { > + accum = accum + ((unsigned char) *source << accum_bits); > + accum_bits += HOST_CHAR_BIT; > + source += 1; > + } > chunk_size = HOST_CHAR_BIT - targ_offset; > if (chunk_size > n) > chunk_size = n; > -- > 2.17.1 -- Joel