Skip site navigation (1) Skip section navigation (2)

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

From: Jukka Holappa <jukkaho(at)mail(dot)student(dot)oulu(dot)fi>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>,pgsql-hackers(at)postgresql(dot)org
Subject: Re: [Resend] Sprintf() auditing and a patch
Date: 2002-08-29 05:15:20
Message-ID: (view raw or flat)
Lists: pgsql-hackers
Hash: SHA1

Neil Conway wrote:
| [ Sorry, never saw the original email ]

Because it is still hanging in moderation queue ;)

| 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,

I tried to use the same style that was used in the code previously.
Apparently I forgot it in some places.

|>>There were also simple mistakes like in
| That code is long dead, BTW.

Well, we'll se what I can dig out of CVS version :) I think string
handling can be very nasty in some places but the problems are so much
easier to find than with integer overflows.

| 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().

I'm sure I did that, because I really didn't know in all places, what
would be the right thing to do.

Using snprintf() there would cause a log message of "using numeric value
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx..." when trying to overflow this. I
agree, being told number to be unpresentable (coming after errorious
string) is not actually necessary when seeing this :)

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

I checked and this is true. However code leaks already in the same
place. (although less bytes).

| Personally, I prefer this:
|         char *buf[1024];

You don't prefer an array of pointers, but I got the point.

|         snprintf(buf, sizeof(buf), "...");
| rather than this:
|         char *buf[1024];
|         snprintf(buf, 1024, "...");
| You used sizeof(...) in some places but not in others.

Very true. I did all my checking and fixing in three nights and didn't
think about the maintainability at first but started using
sizeof(later). I just wanted to get them fixed at first. These should
all be using sizeof(buf) when the target is an array.

There were also places where a simple pointer to a buffer was passed to
another function which then appended some string to it. I think this was
date/time handling in somewhere. That kind of things are impossible to
fix (without changing the function definition) if the appended string
doesn't have a certain maximum size. Dates/times sure have that limit,
but I hope no one copies that code to handle any some variable length

| 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.

All true.:)

- - Jukka
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla -


In response to

pgsql-hackers by date

Next:From: Neil ConwayDate: 2002-08-29 05:27:41
Subject: tweaking MemSet() performance
Previous:From: Bruce MomjianDate: 2002-08-29 05:03:05

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group