This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Tue, Oct 18, 2016 at 06:07:40PM +0200, Lionel Flandrin wrote: > On Tue, Oct 18, 2016 at 11:49:01AM -0400, Simon Marchi wrote: > > On 16-10-18 07:10 AM, Lionel Flandrin wrote: > > > Hello, > > > > > > While investigating an unrelated issue in remote.c I noticed that the > > > bound checking for 'g' packets was bogus: > > > > > > The previous code would only check that the first byte of the register > > > was within bounds before passing the buffer to regcache_raw_supply. If > > > it turned out that the register in the 'g' packet was incomplete then > > > regcache_raw_supply would proceed to memcpy out-of-bounds. > > > > > > Since the buffer is allocated with alloca it's relatively unlikely to > > > crash (you just end up dumping gdb's stack into the cache) but it's > > > still a bit messy. > > > > > > I changed this logic to check for truncated registers and raise an > > > error if one is encountered. Hopefully it should make debugging remote > > > stubs a bit easier. > > > > Hi Lionel, > > > > This patch looks good to me, a few minor comments below about formatting. > > Someone else with the approval stamp must look at it, but hopefully it will > > save them a bit of work. > > Thank you for the feedback, here's the updated patch: > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 4b642b8..3ace874 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,8 @@ > +2016-10-18 Lionel Flandrin <lionel@svkt.org> > + > + * remote.c (process_g_packet): Detect truncated registers in 'g' > + packets and raise an error. > + > 2016-10-18 Maciej W. Rozycki <macro@imgtec.com> > > * i386-tdep.c (i386_mpx_info_bounds): Make sure the architecture > diff --git a/gdb/remote.c b/gdb/remote.c > index af7508a..e1b5ad7 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7163,18 +7163,31 @@ process_g_packet (struct regcache *regcache) > the 'p' packet must be used. */ > if (buf_len < 2 * rsa->sizeof_g_packet) > { > - rsa->sizeof_g_packet = buf_len / 2; > + long sizeof_g_packet = buf_len / 2; > > for (i = 0; i < gdbarch_num_regs (gdbarch); i++) > { > + long offset = rsa->regs[i].offset; > + long reg_size = register_size (gdbarch, i); > + > if (rsa->regs[i].pnum == -1) > continue; > > - if (rsa->regs[i].offset >= rsa->sizeof_g_packet) > + if (offset >= sizeof_g_packet) > rsa->regs[i].in_g_packet = 0; > + else if (offset + reg_size > sizeof_g_packet) > + error (_("Truncated register %d in remote 'g' packet"), i); > else > rsa->regs[i].in_g_packet = 1; > } > + > + /* Looks valid enough, we can assume this is the correct length > + for a 'g' packet. It's important not to adjust > + rsa->sizeof_g_packet if we have truncated registers otherwise > + this "if" won't be run the next time the method is called > + with a packet of the same size and one of the internal errors > + below will trigger instead. */ > + rsa->sizeof_g_packet = sizeof_g_packet; > } > > regs = (char *) alloca (rsa->sizeof_g_packet); > @@ -7204,10 +7217,11 @@ process_g_packet (struct regcache *regcache) > for (i = 0; i < gdbarch_num_regs (gdbarch); i++) > { > struct packet_reg *r = &rsa->regs[i]; > + long reg_size = register_size (gdbarch, i); > > if (r->in_g_packet) > { > - if (r->offset * 2 >= strlen (rs->buf)) > + if ((r->offset + reg_size) * 2 > strlen (rs->buf)) > /* This shouldn't happen - we adjusted in_g_packet above. */ > internal_error (__FILE__, __LINE__, > _("unexpected end of 'g' packet reply")); > I'm politely bumping this so that it doesn't get forgotten. Sorry for the noise. -- Lionel Flandrin
Attachment:
signature.asc
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |