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

-----BEGIN PGP SIGNED MESSAGE-----
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
|>>src/backend/tioga/tgRecipe.c
|>
|
| 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(buf.data) -- 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, "...");
[snip]
| 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
strings..

| 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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9ba3nYYWM2XTSwX0RAoBJAJwK4eA5iPDNaQFF3TCL09MD/dkBwgCdHGmi
b4RCkBnOPBfQMQAX7wJk4U4=
=7hvG
-----END PGP SIGNATURE-----