This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] tracepoint remote.c:remote_trace_set_readonly_regions give up some regions if it is number is too big
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)
{