From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18874 invoked by alias); 15 Jun 2011 07:35:29 -0000 Received: (qmail 18615 invoked by uid 22791); 15 Jun 2011 07:35:27 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-fx0-f41.google.com (HELO mail-fx0-f41.google.com) (209.85.161.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 15 Jun 2011 07:35:02 +0000 Received: by fxm18 with SMTP id 18so240012fxm.0 for ; Wed, 15 Jun 2011 00:35:01 -0700 (PDT) Received: by 10.223.97.196 with SMTP id m4mr197049fan.55.1308123300914; Wed, 15 Jun 2011 00:35:00 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.99.80 with HTTP; Wed, 15 Jun 2011 00:33:46 -0700 (PDT) In-Reply-To: <201106131321.33532.pedro@codesourcery.com> References: <201106131321.33532.pedro@codesourcery.com> From: Hui Zhu Date: Wed, 15 Jun 2011 07:35:00 -0000 Message-ID: Subject: Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2011-06/txt/msg00205.txt.bz2 On Mon, Jun 13, 2011 at 20:21, Pedro Alves wrote: > On Sunday 12 June 2011 15:01:07, Hui Zhu wrote: >> Hi, >> >> My GDB got crash with a ELF file when I use tracepoint with it. =A0I >> found that it have 12898 sections. >> So in remote_trace_set_readonly_regions: >> =A0 =A0 =A0 sprintf (target_buf + strlen (target_buf), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0":%s,%s", tmp1, tmp2); >> It will over write other val, it make GDB crash. >> So I add a check before it to fix it: >> =A0 =A0 =A0 if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 > >> target_buf_size) >> =A0 =A0 =A0 { >> =A0 =A0 =A0 =A0 warning (_("Give up some read only regions.")); >> =A0 =A0 =A0 =A0 break; >> =A0 =A0 =A0 } >> > > Note that if your stub supports qXfer:traceframe-info:read, this packet > is no longer necessary to support, as GDB will handle reading from > readonly sections out of live memory itself. =A0I think this > means that the warning should only be output > if remote_protocol_packets[PACKET_qXfer_traceframe_info].support > is not PACKET_ENABLE? GDB can do that with itself? That is really cool. All this function is implemented inside remote.c? What I suggest is if not need, don't send the QTro. And I suggest we always enable this check inside the function. > >> Please help me review it. >> >> And this issue affect 7.3 too. Does it can check in to 7.3? >> >> Thanks, >> Hui >> >> 2011-06-12 =A0Hui Zhu =A0 >> >> =A0 =A0 =A0 * remote.c (remote_trace_set_readonly_regions): Add a check = for >> =A0 =A0 =A0 target_buf_size. >> --- >> =A0remote.c | =A0 =A05 +++++ >> =A01 file changed, 5 insertions(+) >> >> --- a/remote.c >> +++ b/remote.c >> @@ -9996,6 +9996,11 @@ remote_trace_set_readonly_regions (void) >> =A0 =A0 =A0 =A0size =3D bfd_get_section_size (s); >> =A0 =A0 =A0 =A0sprintf_vma (tmp1, vma); >> =A0 =A0 =A0 =A0sprintf_vma (tmp2, vma + size); > =A0 =A0 if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 > targe= t_buf_size) > > Space before `('. =A0Too long line, split the length calc > into a local out of the if predicate. > > Should be simple to keep the current length of target_buf in > a variable and increment it on each iteration so you don't > need to compute it everytime. > > =A0 =A0 =A0 =A0size =3D bfd_get_section_size (s); > =A0 =A0 =A0 =A0sprintf_vma (tmp1, vma); > =A0 =A0 =A0 =A0sprintf_vma (tmp2, vma + size); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sec_length =3D 1 + strlen (tmp1) + 1 + st= rlen (tmp2); > =A0 =A0 =A0 =A0if (offset + sec_length >=3D target_buf_size) > =A0 =A0 =A0 =A0 =A0{ > =A0 =A0 =A0 =A0 =A0 =A0warning(); > =A0 =A0 =A0 =A0 =A0 =A0break; > =A0 =A0 =A0 =A0 =A0} > > =A0 =A0 =A0 =A0sprintf (target_buf + offset, ":%s,%s", tmp1, tmp2); > =A0 =A0 =A0 =A0offset +=3D sec_length; > >> + =A0 =A0 { >> + =A0 =A0 =A0 warning (_("Give up some read only regions.")); > > Make that: > > "Too many sections for read-only sections definition packet". > >> + =A0 =A0 =A0 break; >> + =A0 =A0 } >> =A0 =A0 =A0 =A0sprintf (target_buf + strlen (target_buf), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0":%s,%s", tmp1, tmp2); >> =A0 =A0 =A0} >> > > -- > Pedro Alves > I make a new patch according to your comments. Please help me review it. Thanks, Hui 2011-06-15 Hui Zhu * remote.c (remote_trace_set_readonly_regions): Add a check for target_buf_size. --- remote.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) --- a/remote.c +++ b/remote.c @@ -9977,6 +9977,7 @@ remote_trace_set_readonly_regions (void) bfd_size_type size; bfd_vma vma; int anysecs =3D 0; + int offset =3D 0; if (!exec_bfd) return; /* No information to give. */ @@ -9985,6 +9986,7 @@ remote_trace_set_readonly_regions (void) for (s =3D exec_bfd->sections; s; s =3D s->next) { char tmp1[40], tmp2[40]; + int sec_length; if ((s->flags & SEC_LOAD) =3D=3D 0 || /* (s->flags & SEC_CODE) =3D=3D 0 || */ @@ -9996,8 +9998,15 @@ remote_trace_set_readonly_regions (void) size =3D bfd_get_section_size (s); sprintf_vma (tmp1, vma); sprintf_vma (tmp2, vma + size); - sprintf (target_buf + strlen (target_buf), - ":%s,%s", tmp1, tmp2); + sec_length =3D 1 + strlen (tmp1) + 1 + strlen (tmp2); + if (offset + sec_length + 1 > target_buf_size) + { + warning (_("\ +Too many sections for read-only sections definition packet.")); + break; + } + sprintf (target_buf + offset, ":%s,%s", tmp1, tmp2); + offset +=3D sec_length; } if (anysecs) {