Thread: (yet) more buffer paranoia
This patches replaces a few more usages of strcpy() and sprintf() when copying into a fixed-size buffer (in this case, a buffer of NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth fixing anyway... Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> This patches replaces a few more usages of strcpy() and sprintf() when
> copying into a fixed-size buffer (in this case, a buffer of
> NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth
> fixing anyway...
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway <neilc@samurai.com> writes:
> This patches replaces a few more usages of strcpy() and sprintf() when
> copying into a fixed-size buffer (in this case, a buffer of
> NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth
> fixing anyway...
I'm not that eager to replace every single sprintf with snprintf.
Most of these are obviously safe and do not need the clutter and
extra cycles...
regards, tom lane
I guess the question is where there are tons more. If not, I think it would be wise to just clean it up so any future uses will look out of place. Can someone check on this? --------------------------------------------------------------------------- Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: > > This patches replaces a few more usages of strcpy() and sprintf() when > > copying into a fixed-size buffer (in this case, a buffer of > > NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth > > fixing anyway... > > I'm not that eager to replace every single sprintf with snprintf. > Most of these are obviously safe and do not need the clutter and > extra cycles... > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I guess the question is where there are tons more. If not, I think it
> would be wise to just clean it up so any future uses will look out of
> place.
Should I point out that Neil already managed to break the regression
tests on the eve of an emergency patch-release with a completely
unnecessary snprintf-ization of show_datestyle?
There *are* risks in changing working code, and while those risks may be
small, I don't see the point of taking them in places where the benefit
is provably zero. If it's not obvious that a sprintf or similar can't
overflow its buffer, then by all means make it snprintf instead. But
I don't hold with the idea that sprintf is ipso facto bad.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
> Neil Conway <neilc@samurai.com> writes:
> > This patches replaces a few more usages of strcpy() and sprintf() when
> > copying into a fixed-size buffer (in this case, a buffer of
> > NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth
> > fixing anyway...
>
> I'm not that eager to replace every single sprintf with snprintf.
> Most of these are obviously safe and do not need the clutter and
> extra cycles...
Hmm, can't seem to see much performance difference:
#include <stdio.h>
int
main(void)
{
char buf[128];
int i;
memset(buf, 0, sizeof(buf));
for (i = 0; i < 1000000; i++)
{
snprintf(buf, sizeof(buf), "%d %d %d %d %d %d %d %d %d %d %d",
i, i + i, i, i + i, i, i + i, i, i + 5, i, i, i);
}
}
With snprintf():
./a.out 7.76s user 0.04s system 99% cpu 7.848 total
./a.out 7.77s user 0.03s system 99% cpu 7.846 total
./a.out 7.70s user 0.09s system 99% cpu 7.867 total
./a.out 7.76s user 0.02s system 99% cpu 7.841 total
./a.out 7.74s user 0.03s system 98% cpu 7.852 total
With sprintf():
./a.out 7.83s user 0.09s system 98% cpu 8.042 total
./a.out 7.89s user 0.04s system 96% cpu 8.247 total
./a.out 7.83s user 0.02s system 98% cpu 7.937 total
./a.out 7.87s user 0.01s system 98% cpu 7.972 total
./a.out 7.80s user 0.01s system 98% cpu 7.918 total
If you're going to make a case for a performance hit, I'd agree that's
a concern -- but I'd prefer some actual data over vague claims of
"extra cycles".
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I guess the question is where there are tons more. If not, I think it > > would be wise to just clean it up so any future uses will look out of > > place. > > Should I point out that Neil already managed to break the regression > tests on the eve of an emergency patch-release with a completely > unnecessary snprintf-ization of show_datestyle? > > There *are* risks in changing working code, and while those risks may be > small, I don't see the point of taking them in places where the benefit > is provably zero. If it's not obvious that a sprintf or similar can't > overflow its buffer, then by all means make it snprintf instead. But > I don't hold with the idea that sprintf is ipso facto bad. Yes, but by changing them, we mark the calls as not having to be reviewed in the future. That seems like a maintenance gain to me. Some of our security patches for 7.2.2 related to sprintf problems, right, so it is a known risk and deserves to be audited. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > This patches replaces a few more usages of strcpy() and sprintf() when > copying into a fixed-size buffer (in this case, a buffer of > NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth > fixing anyway... > > Cheers, > > Neil > > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway writes: > This patches replaces a few more usages of strcpy() and sprintf() when > copying into a fixed-size buffer (in this case, a buffer of > NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth > fixing anyway... Changing strcpy to strncpy is incorrect. strncpy should be banned. -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut wrote: > Neil Conway writes: > > > This patches replaces a few more usages of strcpy() and sprintf() when > > copying into a fixed-size buffer (in this case, a buffer of > > NAMEDATALEN bytes). AFAICT nothing to worry about here, but worth > > fixing anyway... > > Changing strcpy to strncpy is incorrect. strncpy should be banned. Good point. Maybe StrNCpy, which is our standard strncpy(). -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073