Message ID | 20181024162037.21024-1-tom@tromey.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 120244 invoked by alias); 24 Oct 2018 16:21:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 120222 invoked by uid 89); 24 Oct 2018 16:21:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-spam-relays-external:cmsmtp, H*RU:cmsmtp, H*r:cmsmtp, HX-Local-Domain:yes X-HELO: gateway32.websitewelcome.com Received: from gateway32.websitewelcome.com (HELO gateway32.websitewelcome.com) (192.185.145.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Oct 2018 16:21:21 +0000 Received: from cm13.websitewelcome.com (cm13.websitewelcome.com [100.42.49.6]) by gateway32.websitewelcome.com (Postfix) with ESMTP id 1B4A31C121 for <gdb-patches@sourceware.org>; Wed, 24 Oct 2018 11:21:20 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id FLtkgfW6IFxNhFLu4gxMQl; Wed, 24 Oct 2018 11:21:19 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To:MIME-Version :Content-Type:Content-Transfer-Encoding:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: In-Reply-To:References:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=BpnnknWLQOBbmnTq7Jx5SwTlPEw92IWpGU2d+GErsVU=; b=VZdt08D4QQRLou6xtsdm7uVn94 N0njobuuQNukw7vtv+EeB5c5Ravxfp39D3VGxLEew3t9exWZ1G0Hc3sjelEjgb78sRYzryzOjKaAJ jC7OLLFAcFXtgGcxp4eed+G5w; Received: from 97-122-190-66.hlrn.qwest.net ([97.122.190.66]:57706 helo=bapiya.Home) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from <tom@tromey.com>) id 1gFLtj-001mlE-Nn; Wed, 24 Oct 2018 11:20:43 -0500 From: Tom Tromey <tom@tromey.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tom@tromey.com> Subject: [PATCH] Fix buffer overflow in ada-lang.c:move_bits Date: Wed, 24 Oct 2018 10:20:37 -0600 Message-Id: <20181024162037.21024-1-tom@tromey.com> |
Commit Message
Tom Tromey
Oct. 24, 2018, 4:20 p.m. UTC
-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 <tom@tromey.com> * ada-lang.c (move_bits): Don't run off the end of the source buffer. --- gdb/ChangeLog | 5 +++++ gdb/ada-lang.c | 18 ++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-)
Comments
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 <tom@tromey.com> > > * 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" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> I tested your change through our testsuite on the various baremetal
Joel> targets we have, and noticed that it causes regressions on ppc and arm
Joel> targets. It's hopefully something small, but just being back from
Joel> a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
Joel> list to look at further.
Thanks. To reproduce the problem I saw, just rebuild with
-fsanitize=address and run the gdb.ada tests. I don't recall exactly
which ones failed, but you should definitely see a read off the end of
the source buffer.
Tom
On 11/01/2018 03:35 PM, Joel Brobecker wrote: > 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 <tom@tromey.com> >> >> * 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. I was going to suggest that this would benefit from unit tests in the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits? (And maybe move copy_bitwise elsewhere?) Thanks, Pedro Alves
On 11/08/2018 07:11 PM, Pedro Alves wrote: > On 11/01/2018 03:35 PM, Joel Brobecker wrote: >> 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 <tom@tromey.com> >>> >>> * 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. > > I was going to suggest that this would benefit from unit tests in > the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this > exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits? > (And maybe move copy_bitwise elsewhere?) I meant to say dwarf2loc.c instead of dwarf2read.c. Thanks, Pedro Alves
> > I was going to suggest that this would benefit from unit tests in > > the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this > > exactly the same as copy_bitwise? Can we get rid of ada-lang.c:move_bits? > > (And maybe move copy_bitwise elsewhere?) > > I meant to say dwarf2loc.c instead of dwarf2read.c. It does look exactly the same, doesn't it? I'll see if we can just re-use dwarf2loc's copy_bitwise. Thanks for the suggestion!
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;