Thread: Buffer overflow in psql
I'm using Postgrseql 7.4.8. In January, I reported a psql bug. The problem was that an INSERT issued through psql would cause a crash. There was no problem with other operations I tried, or with the same INSERT submitted through JDBC. The discussion thread begins here: http://archives.postgresql.org/pgsql-bugs/2006-01/msg00071.php There was no resolution to this problem -- a bad psql build was suggested and I couldn't disprove it. The problem has occurred again, and I've found a buffer overflow in psql that explains it. Here is code from src/bin/psql/common.c, from the PrintQueryResults function: case PGRES_COMMAND_OK: { char buf[10]; success = true; sprintf(buf, "%u", (unsigned int) PQoidValue(results)); In 8.1.5, the sprintf is replaced by an snprintf, resulting in a less serious form of the bug. I believe that we end up in this code after an INSERT is processed through psql. If PQoidValue returns 1000000000 (1 billion) or higher, which requires 10 characters, then we overflow buf due to the terminal zero. Looking at my databases, I find that the problem occurs exactly in the databases with OIDs above 1 billion for newly inserted rows. (Because the psql crash occurs in processing results, the INSERT succeeds, and I can examine the OIDs of the inserted rows.) My January email indicates that we had been loading data for three months, so OIDs could conceivably have gotten as high as 1 billion (if I understand OIDs correctly). The problem occurred again at about the same time -- three months into a test. Changing the 10 to 11, INSERTs through psql no longer cause psql to crash. I have two questions: 1) Is one of the postgresql developers willing to get this fix into the next release? (We're patching our own 7.4.8 build.) 2) If no one else has hit this, then it suggests I might be in uncharted territory with OIDs getting this high. Do I need to review my vacuuming strategy? (I can summarize my vacuuming strategy for anyone interested.) Jack Orenstein
On Wed, Nov 22, 2006 at 11:11:09AM -0500, Jack Orenstein wrote: > I'm using Postgrseql 7.4.8. In January, I reported a psql bug. The > problem was that an INSERT issued through psql would cause a > crash. There was no problem with other operations I tried, or with the > same INSERT submitted through JDBC. The discussion thread begins here: > http://archives.postgresql.org/pgsql-bugs/2006-01/msg00071.php <snip> > case PGRES_COMMAND_OK: > { > char buf[10]; > > success = true; > sprintf(buf, "%u", (unsigned int) > PQoidValue(results)); > > In 8.1.5, the sprintf is replaced by an snprintf, resulting in a less > serious form of the bug. Looks like you found something. > 1) Is one of the postgresql developers willing to get this fix into > the next release? (We're patching our own 7.4.8 build.) Probably, though I don't know the release cycle for backpatches. > 2) If no one else has hit this, then it suggests I might be in > uncharted territory with OIDs getting this high. Do I need to > review my vacuuming strategy? (I can summarize my vacuuming > strategy for anyone interested.) I think most people have OIDs disabled, which avoids the problem entirely. Perhaps that's why it hasn't been run into before. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
Attachment
Jack Orenstein <jorenstein@archivas.com> writes:
> The problem has occurred again, and I've found a buffer overflow in
> psql that explains it. Here is code from src/bin/psql/common.c, from
> the PrintQueryResults function:
> case PGRES_COMMAND_OK:
> {
> char buf[10];
> success = true;
> sprintf(buf, "%u", (unsigned int) PQoidValue(results));
Good catch! What platform and compiler are you using exactly? I'd
imagine that on most platforms, the size of that array is effectively
rounded up to 12 bytes due to alignment/padding considerations, which
would mask the mistake. Yours must somehow be putting something
critical right after the array.
> 1) Is one of the postgresql developers willing to get this fix into
> the next release? (We're patching our own 7.4.8 build.)
Yeah, we'll fix it.
regards, tom lane
Tom Lane wrote:
> Jack Orenstein <jorenstein@archivas.com> writes:
>> The problem has occurred again, and I've found a buffer overflow in
>> psql that explains it. Here is code from src/bin/psql/common.c, from
>> the PrintQueryResults function:
>
>> case PGRES_COMMAND_OK:
>> {
>> char buf[10];
>
>> success = true;
>> sprintf(buf, "%u", (unsigned int) PQoidValue(results));
>
> Good catch! What platform and compiler are you using exactly? I'd
> imagine that on most platforms, the size of that array is effectively
> rounded up to 12 bytes due to alignment/padding considerations, which
> would mask the mistake. Yours must somehow be putting something
> critical right after the array.
We're using gcc-4.0.2-8.fc4 on FC4 (intel). I believe that we didn't
just get lucky with the overflow. One of our Linux experts says that
our libc is doing memory bounds checking. Note that the stack
goes through __sprintf_chk:
/lib/libc.so.6(__chk_fail+0x41)[0xb7c0bbc5]
/lib/libc.so.6(__vsprintf_chk+0x0)[0xb7c0b490]
/lib/libc.so.6(_IO_default_xsputn+0x97)[0xb7b8e8d8]
/lib/libc.so.6(_IO_vfprintf+0x1aeb)[0xb7b6a2f7]
/lib/libc.so.6(__vsprintf_chk+0xa1)[0xb7c0b531]
/lib/libc.so.6(__sprintf_chk+0x30)[0xb7c0b484]
/usr/bin/psql[0x804ea63]
/usr/bin/psql[0x805195b]
/usr/bin/psql[0x804afdd]
/usr/bin/psql[0x804cde9]
/usr/bin/psql[0x804deb5]
/usr/bin/psql[0x80510cf]
/usr/bin/psql[0x805336d]
/lib/libc.so.6(__libc_start_main+0xdf)[0xb7b42d7f]
/usr/bin/psql[0x804a8e1]
Our Linux kernel is 2.6.17-1.2139
Jack Orenstein
Jack Orenstein <jorenstein@Archivas.com> writes:
> Tom Lane wrote:
>> Good catch! What platform and compiler are you using exactly? I'd
>> imagine that on most platforms, the size of that array is effectively
>> rounded up to 12 bytes due to alignment/padding considerations, which
>> would mask the mistake. Yours must somehow be putting something
>> critical right after the array.
> We're using gcc-4.0.2-8.fc4 on FC4 (intel). I believe that we didn't
> just get lucky with the overflow. One of our Linux experts says that
> our libc is doing memory bounds checking.
Ah so, that explains how come it noticed. BTW, I see that somebody
already changed the array size to 16 bytes in HEAD --- so it's just
the back branches that need fixing.
regards, tom lane
Tom Lane wrote: > Ah so, that explains how come it noticed. BTW, I see that somebody > already changed the array size to 16 bytes in HEAD --- so it's just > the back branches that need fixing. Um, is that really considered a fix??? We all know that there's no guarantee at all, even in ANSI C, that unsigned int isn't bigger than 32 bits, right? There are still some weird architectures out there. Whenever I need to print some integer x, I use code like this: char buf[1 + sizeof(x) * CHAR_BIT / 3] I let the compiler figure out the length needed to print in octal, and use that as a (slight) over-estimate of the length for decimal. As a bonus, the type of x can be changed without having to track down this kind of crap. Alternatively, the code in question could just cast to one of the newer fixed-length int types, like int32_t, although that has its own problems. Sorry for the pedantry ... - John D. Burger MITRE
"John D. Burger" <john@mitre.org> writes:
> Tom Lane wrote:
>> Ah so, that explains how come it noticed. BTW, I see that somebody
>> already changed the array size to 16 bytes in HEAD --- so it's just
>> the back branches that need fixing.
> Um, is that really considered a fix??? We all know that there's no
> guarantee at all, even in ANSI C, that unsigned int isn't bigger than
> 32 bits, right?
OID is 32 bits. Full stop.
regards, tom lane
Tom Lane wrote: >> Um, is that really considered a fix??? We all know that there's no >> guarantee at all, even in ANSI C, that unsigned int isn't bigger than >> 32 bits, right? > > OID is 32 bits. Full stop. I should know better than to argue about this, but: In that case, casting it as in the OP's code sample seems problematic in the other direction: sprintf(buf, "%u", (unsigned int)PQoidValue(results)); since unsigned int could be as small as 16 bits, thus truncating the OID value. Ok, I'll stop now, I promise. - John D. Burger MITRE