[RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Hui Zhu]

Hi,

My GDB got crash with a ELF file when I use tracepoint with it.  I
found that it have 12898 sections.
So in remote_trace_set_readonly_regions:
      sprintf (target_buf + strlen (target_buf),
	       ":%s,%s", tmp1, tmp2);
It will over write other val, it make GDB crash.
So I add a check before it to fix it:
      if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 >
target_buf_size)
	{
	  warning (_("Give up some read only regions."));
	  break;
	}

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  Hui Zhu  <teawater@gmail.com>

	* remote.c (remote_trace_set_readonly_regions): Add a check for
	target_buf_size.
---
 remote.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/remote.c
+++ b/remote.c
@@ -9996,6 +9996,11 @@ remote_trace_set_readonly_regions (void)
       size = bfd_get_section_size (s);
       sprintf_vma (tmp1, vma);
       sprintf_vma (tmp2, vma + size);
+      if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 >
target_buf_size)
+	{
+	  warning (_("Give up some read only regions."));
+	  break;
+	}
       sprintf (target_buf + strlen (target_buf),
 	       ":%s,%s", tmp1, tmp2);
     }

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Pedro Alves]

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.  I
> found that it have 12898 sections.
> So in remote_trace_set_readonly_regions:
>       sprintf (target_buf + strlen (target_buf),
> 	       ":%s,%s", tmp1, tmp2);
> It will over write other val, it make GDB crash.
> So I add a check before it to fix it:
>       if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 >
> target_buf_size)
> 	{
> 	  warning (_("Give up some read only regions."));
> 	  break;
> 	}
> 

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.  I think this
means that the warning should only be output
if remote_protocol_packets[PACKET_qXfer_traceframe_info].support
is not PACKET_ENABLE?

> 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  Hui Zhu  <teawater@gmail.com>
> 
> 	* remote.c (remote_trace_set_readonly_regions): Add a check for
> 	target_buf_size.
> ---
>  remote.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/remote.c
> +++ b/remote.c
> @@ -9996,6 +9996,11 @@ remote_trace_set_readonly_regions (void)
>        size = bfd_get_section_size (s);
>        sprintf_vma (tmp1, vma);
>        sprintf_vma (tmp2, vma + size);
     if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 > target_buf_size)

Space before `('.  Too 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.

        size = bfd_get_section_size (s);
        sprintf_vma (tmp1, vma);
        sprintf_vma (tmp2, vma + size);
		 sec_length = 1 + strlen (tmp1) + 1 + strlen (tmp2);
        if (offset + sec_length >= target_buf_size)
          {
            warning();
            break;
          }

        sprintf (target_buf + offset, ":%s,%s", tmp1, tmp2);
        offset += sec_length;

> +	{
> +	  warning (_("Give up some read only regions."));

Make that:

"Too many sections for read-only sections definition packet".

> +	  break;
> +	}
>        sprintf (target_buf + strlen (target_buf),
>  	       ":%s,%s", tmp1, tmp2);
>      }
> 

-- 
Pedro Alves

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Hui Zhu]

On Mon, Jun 13, 2011 at 20:21, Pedro Alves <pedro@codesourcery.com> 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.  I
>> found that it have 12898 sections.
>> So in remote_trace_set_readonly_regions:
>>       sprintf (target_buf + strlen (target_buf),
>>              ":%s,%s", tmp1, tmp2);
>> It will over write other val, it make GDB crash.
>> So I add a check before it to fix it:
>>       if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 >
>> target_buf_size)
>>       {
>>         warning (_("Give up some read only regions."));
>>         break;
>>       }
>>
>
> 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.  I 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  Hui Zhu  <teawater@gmail.com>
>>
>>       * remote.c (remote_trace_set_readonly_regions): Add a check for
>>       target_buf_size.
>> ---
>>  remote.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -9996,6 +9996,11 @@ remote_trace_set_readonly_regions (void)
>>        size = bfd_get_section_size (s);
>>        sprintf_vma (tmp1, vma);
>>        sprintf_vma (tmp2, vma + size);
>     if (strlen (target_buf) + strlen(tmp1) + strlen(tmp2) + 3 > target_buf_size)
>
> Space before `('.  Too 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.
>
>        size = bfd_get_section_size (s);
>        sprintf_vma (tmp1, vma);
>        sprintf_vma (tmp2, vma + size);
>                 sec_length = 1 + strlen (tmp1) + 1 + strlen (tmp2);
>        if (offset + sec_length >= target_buf_size)
>          {
>            warning();
>            break;
>          }
>
>        sprintf (target_buf + offset, ":%s,%s", tmp1, tmp2);
>        offset += sec_length;
>
>> +     {
>> +       warning (_("Give up some read only regions."));
>
> Make that:
>
> "Too many sections for read-only sections definition packet".
>
>> +       break;
>> +     }
>>        sprintf (target_buf + strlen (target_buf),
>>              ":%s,%s", tmp1, tmp2);
>>      }
>>
>
> --
> Pedro Alves
>

I make a new patch according to your comments.  Please help me review it.

Thanks,
Hui

2011-06-15  Hui Zhu  <teawater@gmail.com>

	* 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 = 0;
+  int offset = 0;

   if (!exec_bfd)
     return;			/* No information to give.  */
@@ -9985,6 +9986,7 @@ remote_trace_set_readonly_regions (void)
   for (s = exec_bfd->sections; s; s = s->next)
     {
       char tmp1[40], tmp2[40];
+      int sec_length;

       if ((s->flags & SEC_LOAD) == 0 ||
       /*  (s->flags & SEC_CODE) == 0 || */
@@ -9996,8 +9998,15 @@ remote_trace_set_readonly_regions (void)
       size = 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 = 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 += sec_length;
     }
   if (anysecs)
     {

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Pedro Alves]

On Wednesday 15 June 2011 08:33:46, Hui Zhu wrote:
> On Mon, Jun 13, 2011 at 20:21, Pedro Alves <pedro@codesourcery.com> wrote:
> > 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.  I 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?

No, it's in:

   target.c:memory_xfer_partial
     target.c:traceframe_available_memory
     target.c:memory_xfer_live_readonly_partial

> What I suggest is if not need, don't send the QTro.

Could work too.  It would still be the same 
remote_protocol_packets[PACKET_qXfer_traceframe_info].support
check...

> And I suggest we always enable this check inside the function.

That was never in question.  I was only talking about silencing
the _warning_.

> 2011-06-15  Hui Zhu  <teawater@gmail.com>
> 
> 	* remote.c (remote_trace_set_readonly_regions): Add a check for
> 	target_buf_size.

Okay.

-- 
Pedro Alves

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Hui Zhu]

On Wed, Jun 15, 2011 at 18:27, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 15 June 2011 08:33:46, Hui Zhu wrote:
>> On Mon, Jun 13, 2011 at 20:21, Pedro Alves <pedro@codesourcery.com> wrote:
>> > 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.  I 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?
>
> No, it's in:
>
>   target.c:memory_xfer_partial
>     target.c:traceframe_available_memory
>     target.c:memory_xfer_live_readonly_partial
>
>> What I suggest is if not need, don't send the QTro.
>
> Could work too.  It would still be the same
> remote_protocol_packets[PACKET_qXfer_traceframe_info].support
> check...
>
>> And I suggest we always enable this check inside the function.
>
> That was never in question.  I was only talking about silencing
> the _warning_.

Sorry for misunderstand your mean.  I post a new patch for it.  Please
help me review it.

>
>> 2011-06-15  Hui Zhu  <teawater@gmail.com>
>>
>>       * remote.c (remote_trace_set_readonly_regions): Add a check for
>>       target_buf_size.
>
> Okay.

Checked in to 7.3 and trunk.

>
> --
> Pedro Alves
>

Thanks,
Hui

2011-06-15  Hui Zhu  <teawater@gmail.com>

	* remote.c (remote_trace_set_readonly_regions): Add check for
	remote_protocol_packets[PACKET_qXfer_traceframe_info].support before
	output warning.
---
 remote.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/remote.c
+++ b/remote.c
@@ -10001,7 +10001,9 @@ remote_trace_set_readonly_regions (void)
       sec_length = 1 + strlen (tmp1) + 1 + strlen (tmp2);
       if (offset + sec_length + 1 > target_buf_size)
 	{
-	  warning (_("\
+	  if (remote_protocol_packets[PACKET_qXfer_traceframe_info].support
+		!= PACKET_ENABLE)
+	    warning (_("\
 Too many sections for read-only sections definition packet."));
 	  break;
 	}

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Pedro Alves]

On Wednesday 15 June 2011 16:52:52, Hui Zhu wrote:
> 2011-06-15  Hui Zhu  <teawater@gmail.com>
> 
>         * remote.c (remote_trace_set_readonly_regions): Add check for
>         remote_protocol_packets[PACKET_qXfer_traceframe_info].support before
>         output warning.

Okay, thanks.

-- 
Pedro Alves

Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big

[Hui Zhu]

Checked in to 7.3 and trunk.

Thanks Pedro.

Best,
Hui

On Thu, Jun 16, 2011 at 00:26, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 15 June 2011 16:52:52, Hui Zhu wrote:
>> 2011-06-15  Hui Zhu  <teawater@gmail.com>
>>
>>         * remote.c (remote_trace_set_readonly_regions): Add check for
>>         remote_protocol_packets[PACKET_qXfer_traceframe_info].support before
>>         output warning.
>
> Okay, thanks.
>
> --
> Pedro Alves
>