Re: [Resend] Sprintf() auditing and a patch

[ Sorry, never saw the original email ]

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Jukka Holappa wrote:
> > I'm very new to this project and inspired by recent security
> > release, I started to audit postgresql source against common
> > mistakes with sprintf().

If you're interested, another common source of problems is integer
overflow when dealing with numeric input from the user. In fact, far
more security problems have been caused by insufficient integer
overflow checking than by string handling bugs.

FYI, we prefer patches in context diff format (diff -c). Also, there
are some code style rules that most of the backend code follows. For
example,

for (i = 0; i < x; i++) { ....

rather than:

for(i=0;i<x;++i) {

And indented using tabs. In any case, these should be automatically
corrected by Bruce before a release is made, but it would be nice if
patches followed this style.

> > There were also simple mistakes like in
> > src/backend/tioga/tgRecipe.c

That code is long dead, BTW.

> > Some of my fixes cause code to be a bit slower because of
> > dynamically allocated mem

Given that you're not using StringInfo in any performance-critical
areas AFAICT (mostly in contrib/, for example), I would suspect the
performance difference wouldn't be too steep (although it's worth
verifying that before the patch is applied). I briefly benchmarked
snprintf() versus sprintf() a couple days ago and found no performance
difference, but using StringInfo may impose a higher penalty.

I'd agree that StringInfo is appropriate when the string is frequently
being appended to (and the code using the strlen() pointer arithmetic
technique you mentioned); however, you've converted the code to use
StringInfo on situations in which it is clearly not warranted. To pick
one example at random, seg_atof(char *) in contrib/seg/segparse.y
doesn't require anything more than a statically sized buffer and
snprintf().

Also, that routine happens to leak memory, since you forgot to call
pfree(buf.data) -- I believe you made the same mistake in several
other places, such as seg_yyerror(char *) in the same file.

Personally, I prefer this:

char *buf[1024];

snprintf(buf, sizeof(buf), "...");

rather than this:

char *buf[1024];

snprintf(buf, 1024, "...");

(even if the size of the char array is a preprocessor constant).

The reason being that

(a) it is more clear: the code plainly states "write to this
string, up to the declared size of the string but no
more".

(b) it is more maintainable: if someone were to change the
size of the char array to, say, 512 bytes but didn't
change the snprintf(), you'd have a potential bug.

You used sizeof(...) in some places but not in others.

That's all I noticed briefly eye-balling the patch; please re-diff
against CVS HEAD and submit a context diff and I'll take another look.

> > Please take look at this patch but since I have worked three long
> > nights with this one, there probably are bugs. I tried compiling
> > it with "configure --with-tcl --with-perl --with-python" and at
> > least it compiled for me :) But that's about all I can promise.

FYI, running the regression tests is an easy way to do some basic
testing. Since code that causes regression tests to fail won't be
accepted (period), you may as well run them now, rather now later.

> > At least I didn't just bitch and moan about the bugs. ;)

Thank you; frankly, I wish your attitude was more common.

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC