Re: Buffer Overflow Patch in GDB/TCL Interface code.

[Fernando Nasser <fnasser at cygnus dot com>]

Steven Johnson wrote:
> 
> There are a number of fixed size buffers defined in the C code that acts as an
> interface between GDB and the TCL Insight. Unfortunately, on my system some of
> those buffers were not big enough, leading to stack corruption and a resultant
> segfault. I Specifically had a problem with clearing breakpoints, the problem
> was that the path to my source file was quite long, and caused a sprintf to
> overflow it's pre-allocated buffer.
> 
> Instead of just adding some more to the buffer, i decided to fix as many of the
> pre-allocated buffers as possible, so that this sort of problem can not occur
> in future.
> 
> A Couple of fixes were tried. The one I settled on was wholescale replacement
> of as many 'sprintf'/'vsprintf' calls with 'asprintf' and 'vasprintf'
> respectively, as was possible without serious modification of the existing
> code. There are now only 2 or 3 places in gdbtk-cmds where sprintf is used
> (Mainly to do with source file loading). Everywhere else has been replaced with
> asprintf/vasprintf.
> 
> Given that neither asprintf or vasprintf are in the MAN pages of linux, here is
> a brief explenation.
> 
> asprintf (I call it "auto sprintf"): It automatically allocates a buffer of the
> correct size to hold the sprintf'd string. The buffer when finished with must
> be discarded with "free", otherwise you will get a heap leak.
> 
> vasprintf is the same as vsprintf, except it too automatically allocates its
> buffer.
> 
> Eg:
> 
> Where code would have once been:
> 
> char buf[1024]; /* This is big enough for any reasonable use I reckon. */
> sprintf(buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);
> 
> it is written as:
> 
> char *buf;
> sprintf(&buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);
> 
> /* I have now finished with buf, so... */
> free(buf);
> 
> The big difference is you no longer have to guess the size of your buffer, so
> no more stack corruption can result.
> 
> It has been used elsewhere in GDB/Insight (remote.c and MI) to name a couple,
> ergo, "it's portable". If it isn't really portable, then I would suggest adding
> a local copy to utils.c in GDB for targets that do not support it.
> 
> Anyway, here comes the changelog and patch:
(omitted for brevity)


That is wonderful Steven.  Thank you very much.  I will check this in as soon as possible.

P.S.: I will have to create a ChangeLog entry in the right format first (of course you 
couldn't know as the Insight "how to contribute" is not ready yet).  It is the same
format for all GNU stuff.  It goes like this:
<Date>  <name> <email>
    * <filename> (<function-list>): <description>
I will apply your patch to my sources and them use the "cvs diff -p" to get (most of)
the function names (some you have to look at the file).


P.S.2: You found another bug (or sort of).  The gdbtk-variable.c is not used anymore.
       It was replaced by gdbtk-varobj.c (and some files in the gdb core: wrapper.c,
       varobj.c).  I will delete it from the current cvs sources.  Sorry about that.



-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9