Re: [Resend] Sprintf() auditing and a patch - Mailing list pgsql-hackers
From | Jukka Holappa |
---|---|
Subject | Re: [Resend] Sprintf() auditing and a patch |
Date | |
Msg-id | 3D6DADE8.9000602@mail.student.oulu.fi Whole thread Raw |
In response to | Re: [Resend] Sprintf() auditing and a patch (Bruce Momjian <pgman@candle.pha.pa.us>) |
List | pgsql-hackers |
-----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-----
pgsql-hackers by date: