Re: [Resend] Sprintf() auditing and a patch
От | Neil Conway |
---|---|
Тема | Re: [Resend] Sprintf() auditing and a patch |
Дата | |
Msg-id | 874rdexr28.fsf@mailbox.samurai.com обсуждение исходный текст |
Ответ на | Re: [Resend] Sprintf() auditing and a patch (Bruce Momjian <pgman@candle.pha.pa.us>) |
Ответы |
Re: [Resend] Sprintf() auditing and a patch
|
Список | pgsql-hackers |
[ Sorry, never saw the original email ] Bruce Momjian <pgman@candle.pha.pa.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 stringbut no more". (b) it is more maintainable: if someone were to change the size of the char array to, say, 512 bytes butdidn'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@samurai.com> || PGP Key ID: DB3C29FC
В списке pgsql-hackers по дате отправления: