Re: potential hazard in Str.c++

[hugh@mimosa.com ("D. Hugh Redelmeier")]

| From: Michael Douglass <mikedoug@texas.net>
| Subject: potential hazard in Str.c++
| 
| There are two big potential problems with fxStr::format and
| fxStr::vformat.  Copiees of the functions are supplied here.
| This comes from hylafax v4.0p1
| 
| First of all is the potential (but not what prompted me to write) for a
| buffer-overflow.  You may want to check for the existance of vsnprintf
| or __vsnprintf and use those in place of vsprintf.

I agree that I would not have written the code this way.  On the other
hand, there isn't yet a convenient library function that would format
a string into a bounded buffer.  I guess that 4096 is a fairly safe
upper bound.

In my own code, I make sure that all my formats have provable upper
bounds.  For example, I write something like %100s instead of %s.

| Secondly, and most importantly, is that you are returning a pointer
| to an automatic buffer which is not guarunteed to be there any longer.
| In fact, certain programming practices in C++ make it a pretty darned
| good reality that they won't exist!  Never return the address of
| an automatic variable--the value, sure--but never the address.  This
| space either needs to have static allocation or dynamic allocation that
| must then be freed later.
| 
| fxStr
| fxStr::format(const char* fmt ...)
| {
|     char buf[4096];
|     va_list ap;
|     va_start(ap, fmt);
|     vsprintf(buf, fmt, ap);
|     va_end(ap);
|     return fxStr(buf);
| }
| 
| fxStr
| fxStr::vformat(const char* fmt, va_list ap)
| {
|     char buf[4096];
|     vsprintf(buf, fmt, ap);
|     return fxStr(buf);
| }

I don't really know C++, but the way I read this code, the return
statements are calling a constructor (fxStr::fxStr(const char *s)).
This constructor allocates memory for the buffer on the heap (new
char[l])and copies in the parameter.  It also builds an anonymous
fxStr object in a temp in the caller (format or vformat) and returns.
The return statements of the caller then return this newly constructed
object (by value).  I don't think that there is any dangling pointer.

BTW, in the class definition (in util/Str.h), the constructor is
declared:
	fxStr(char const *s)
But in the definition of the constructor (in util/Str.c++), it is
declared:
	fxStr(const char *s)
I find the former odd, and I find it even more odd to make them
different.

Hugh Redelmeier
hugh@mimosa.com  voice: +1 416 482-8253