This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
[RFA] Remove some sprintfs from vCont client support
- From: Daniel Jacobowitz <drow at mvista dot com>
- To: gdb-patches at sources dot redhat dot com
- Date: Tue, 2 Dec 2003 11:07:04 -0500
- Subject: [RFA] Remove some sprintfs from vCont client support
- References: <20030929152831.GA23286@nevyn.them.org> <20030930211717.GB19869@nevyn.them.org> <3F8C917C.1080708@gnu.org> <20031016203156.GA24204@nevyn.them.org> <3F8F0B2B.9080506@redhat.com> <20031016221433.GA553@nevyn.them.org> <3F8F1B3B.7000904@redhat.com> <20031016225328.GA1542@nevyn.them.org>
On Thu, Oct 16, 2003 at 06:53:28PM -0400, Daniel Jacobowitz wrote:
> On Thu, Oct 16, 2003 at 06:27:07PM -0400, Andrew Cagney wrote:
> > >There are two probems:
> > >>
> > >>- the buffer can get very very large and that can blow the stack
If that concerns you then I suggest the alloca's in putpkt_binary,
which I found while fixing this. That's a whole lot more worrisome,
since there's one as big as the packet plus one as big as the max
packet length.
> > >>- it isn't possible to audit this code (with out a deep understanding of
> > >>that value) and hence demonstrate that the sprintf won't smash the
> > >>stack/heap
> > >>
> > >>You'll need to also change the sprintf to snprintf (parameterized with
> > >>remote_packet_size.
> > >
> > >
> > >I don't see a point in doing that until someone expresses interest in
> > >thread locking or some other feature which requires adding to the code.
> > >The maximum length of any generated vcont packet is the length of:
> > > vCont;C01:12341468;C02
> > >The minimum possible buffer size is about twenty times that.
> >
> > I wrote "it isn't possible to audit this code (with out a deep
> > understanding of that [remote_packet_size] value)". The code should be
> > locally robust.
>
> I wouldn't call the minimum size a deep understanding. It isn't
> documented anywhere in the code but I think that it should be.
>
> But I'll fix it next week.
OK, so it wasn't the next week. I took advantage of the new xstrprintf
function to get something I'm happy with. How about you, Andrew - is
this patch OK?
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
2003-12-02 Daniel Jacobowitz <drow@mvista.com>
* remote.c (remote_vcont_resume): Use xstrprintf instead of sprintf.
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.122
diff -u -p -r1.122 remote.c
--- remote.c 10 Nov 2003 21:20:44 -0000 1.122
+++ remote.c 2 Dec 2003 15:57:33 -0000
@@ -2578,7 +2578,7 @@ remote_vcont_resume (ptid_t ptid, int st
{
struct remote_state *rs = get_remote_state ();
int pid = PIDGET (ptid);
- char *buf = NULL;
+ char *buf = NULL, *outbuf;
struct cleanup *old_cleanup;
buf = xmalloc (rs->remote_packet_size);
@@ -2603,40 +2603,45 @@ remote_vcont_resume (ptid_t ptid, int st
don't have any PID numbers the inferior will understand. Make sure
to only send forms that do not specify a PID. */
if (step && siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;S%02x", siggnal);
+ outbuf = xstrprintf ("vCont;S%02x", siggnal);
else if (step)
- sprintf (buf, "vCont;s");
+ outbuf = xstrprintf ("vCont;s");
else if (siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;C%02x", siggnal);
+ outbuf = xstrprintf ("vCont;C%02x", siggnal);
else
- sprintf (buf, "vCont;c");
+ outbuf = xstrprintf ("vCont;c");
}
else if (pid == -1)
{
/* Resume all threads, with preference for INFERIOR_PTID. */
if (step && siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;S%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+ outbuf = xstrprintf ("vCont;S%02x:%x;c", siggnal,
+ PIDGET (inferior_ptid));
else if (step)
- sprintf (buf, "vCont;s:%x;c", PIDGET (inferior_ptid));
+ outbuf = xstrprintf ("vCont;s:%x;c", PIDGET (inferior_ptid));
else if (siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;C%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+ outbuf = xstrprintf ("vCont;C%02x:%x;c", siggnal,
+ PIDGET (inferior_ptid));
else
- sprintf (buf, "vCont;c");
+ outbuf = xstrprintf ("vCont;c");
}
else
{
/* Scheduler locking; resume only PTID. */
if (step && siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;S%02x:%x", siggnal, pid);
+ outbuf = xstrprintf ("vCont;S%02x:%x", siggnal, pid);
else if (step)
- sprintf (buf, "vCont;s:%x", pid);
+ outbuf = xstrprintf ("vCont;s:%x", pid);
else if (siggnal != TARGET_SIGNAL_0)
- sprintf (buf, "vCont;C%02x:%x", siggnal, pid);
+ outbuf = xstrprintf ("vCont;C%02x:%x", siggnal, pid);
else
- sprintf (buf, "vCont;c:%x", pid);
+ outbuf = xstrprintf ("vCont;c:%x", pid);
}
- putpkt (buf);
+ gdb_assert (outbuf && strlen (outbuf) < rs->remote_packet_size);
+ make_cleanup (xfree, outbuf);
+
+ putpkt (outbuf);
do_cleanups (old_cleanup);