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

[Hui Zhu <teawater@gmail.com>]

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)
     {