BUG #1044: snprintf() shipped with PostgreSQL is not thread safe
The following bug has been logged online:
Bug reference: 1044
Logged by: Denis Stepanov
Email address: D.N.Stepanov@inp.nsk.su
PostgreSQL version: 7.4
Operating system: Any OS lacking proper snprintf()
Description: snprintf() shipped with PostgreSQL is not thread safe
Details:
Some OSes lack proper snprintf()/vsnprintf() fuctions so PostgreSQL includes
its own version (src/port/snprintf.c) during building. Unfortunately, this
version of snprintf() is not reentrant (it uses global vars to keep internal
state), so for example running libpq-based concurrent applications (threads)
causes libpq fuctions to fail sometimes. The suggestion is to remove globals
usage in src/port/snprintf.c. I am attaching here a sample patch for your
review. Thanks.
--- postgresql-7.4.1/src/port/snprintf.c Thu Jul 18 11:13:59 2002
+++ postgresql-7.4.1.fixed/src/port/snprintf.c Thu Jan 8 13:23:47 2004
@@ -67,6 +67,9 @@
* Sigh. This sort of thing is always nasty do deal with. Note that
* the version here does not include floating point. (now it does ... tgl)
*
+ * Denis Stepanov Thu Jan 8 13:01:01 NOVT 2004
+ * Made functions reentrant (thread safe).
+ *
* snprintf() is used instead of sprintf() as it does limit checks
* for string length. This covers a nasty loophole.
*
@@ -75,12 +78,18 @@
**************************************************************/ /*static char _id[] = "$Id: snprintf.c,v 1.1 2002/07/18 04:13:59 momjian
Exp $";*/
-static char *end;
-static int SnprfOverflow;
+
+typedef struct _vsnprintf_data
+{
+ int SnprfOverflow;
+ char *end;
+ char *output;
+
+} vsnprintf_data; int snprintf(char *str, size_t count, const char *fmt,...);
int vsnprintf(char *str, size_t count, const char *fmt, va_list args);
-static void dopr(char *buffer, const char *format, va_list args);
+static void dopr(char *buffer, const char *format, va_list args,
vsnprintf_data *vsnpd); int
snprintf(char *str, size_t count, const char *fmt,...)
@@ -98,12 +107,14 @@
int
vsnprintf(char *str, size_t count, const char *fmt, va_list args)
{
+ vsnprintf_data vsnpd;
+
str[0] = '\0';
- end = str + count - 1;
- SnprfOverflow = 0;
- dopr(str, fmt, args);
+ vsnpd.end = str + count - 1;
+ vsnpd.SnprfOverflow = 0;
+ dopr(str, fmt, args, &vsnpd);
if (count > 0)
- end[0] = '\0';
+ vsnpd.end[0] = '\0';
return strlen(str);
}@@ -111,17 +122,16 @@
* dopr(): poor man's version of doprintf
*/
-static void fmtstr(char *value, int ljust, int len, int zpad, int
maxwidth);
-static void fmtnum(long_long value, int base, int dosign, int ljust, int
len, int zpad);
-static void fmtfloat(double value, char type, int ljust, int len, int
precision, int pointflag);
-static void dostr(char *str, int cut);
-static void dopr_outch(int c);
+static void fmtstr(char *value, int ljust, int len, int zpad, int maxwidth,
vsnprintf_data *vsnpd);
+static void fmtnum(long_long value, int base, int dosign, int ljust, int
len, int zpad, vsnprintf_data *vsnpd);
+static void fmtfloat(double value, char type, int ljust, int len, int
precision, int pointflag, vsnprintf_data *vsnpd);
+static void dostr(char *str, int cut, vsnprintf_data *vsnpd);
+static void dopr_outch(int c, vsnprintf_data *vsnpd);-static char *output;
static void
-dopr(char *buffer, const char *format, va_list args)
+dopr(char *buffer, const char *format, va_list args, vsnprintf_data *vsnpd)
{
int ch;
long_long value;
@@ -135,7 +145,7 @@
int len;
int zpad;- output = buffer;
+ vsnpd->output = buffer;
while ((ch = *format++))
{
switch (ch)
@@ -148,8 +158,8 @@
switch (ch)
{
case 0:
- dostr("**end of format**", 0);
- *output = '\0';
+ dostr("**end of format**", 0, vsnpd);
+ *vsnpd->output = '\0';
return;
case '-':
ljust = 1;
@@ -188,7 +198,7 @@
goto nextch;
case 'u':
case 'U':
- /* fmtnum(value,base,dosign,ljust,len,zpad) */
+ /* fmtnum(value,base,dosign,ljust,len,zpad,vsnpd) */
if (longflag)
{
if (longlongflag)
@@ -198,11 +208,11 @@
}
else
value = va_arg(args, unsigned int);
- fmtnum(value, 10, 0, ljust, len, zpad);
+ fmtnum(value, 10, 0, ljust, len, zpad, vsnpd);
break;
case 'o':
case 'O':
- /* fmtnum(value,base,dosign,ljust,len,zpad) */
+ /* fmtnum(value,base,dosign,ljust,len,zpad,vsnpd) */
if (longflag)
{
if (longlongflag)
@@ -212,7 +222,7 @@
}
else
value = va_arg(args, unsigned int);
- fmtnum(value, 8, 0, ljust, len, zpad);
+ fmtnum(value, 8, 0, ljust, len, zpad, vsnpd);
break;
case 'd':
case 'D':
@@ -225,7 +235,7 @@
}
else
value = va_arg(args, int);
- fmtnum(value, 10, 1, ljust, len, zpad);
+ fmtnum(value, 10, 1, ljust, len, zpad, vsnpd);
break;
case 'x':
if (longflag)
@@ -237,7 +247,7 @@
}
else
value = va_arg(args, unsigned int);
- fmtnum(value, 16, 0, ljust, len, zpad);
+ fmtnum(value, 16, 0, ljust, len, zpad, vsnpd);
break;
case 'X':
if (longflag)
@@ -249,7 +259,7 @@
}
else
value = va_arg(args, unsigned int);
- fmtnum(value, -16, 0, ljust, len, zpad);
+ fmtnum(value, -16, 0, ljust, len, zpad, vsnpd);
break;
case 's':
strvalue = va_arg(args, char *);
@@ -257,12 +267,12 @@
{
if (pointflag && len > maxwidth)
len = maxwidth; /* Adjust padding */
- fmtstr(strvalue, ljust, len, zpad, maxwidth);
+ fmtstr(strvalue, ljust, len, zpad, maxwidth, vsnpd);
}
break;
case 'c':
ch = va_arg(args, int);
- dopr_outch(ch);
+ dopr_outch(ch, vsnpd);
break;
case 'e':
case 'E':
@@ -270,25 +280,25 @@
case 'g':
case 'G':
fvalue = va_arg(args, double);
- fmtfloat(fvalue, ch, ljust, len, maxwidth, pointflag);
+ fmtfloat(fvalue, ch, ljust, len, maxwidth, pointflag, vsnpd);
break;
case '%':
- dopr_outch(ch);
+ dopr_outch(ch, vsnpd);
continue;
default:
- dostr("???????", 0);
+ dostr("???????", 0, vsnpd);
}
break;
default:
- dopr_outch(ch);
+ dopr_outch(ch, vsnpd);
break;
}
}
- *output = '\0';
+ *vsnpd->output = '\0';
} static void
-fmtstr(char *value, int ljust, int len, int zpad, int maxwidth)
+fmtstr(char *value, int ljust, int len, int zpad, int maxwidth,
vsnprintf_data *vsnpd)
{
int padlen,
strlen; /* amount to pad */
@@ -305,19 +315,19 @@
padlen = -padlen;
while (padlen > 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
--padlen;
}
- dostr(value, maxwidth);
+ dostr(value, maxwidth, vsnpd);
while (padlen < 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
++padlen;
}
} static void
-fmtnum(long_long value, int base, int dosign, int ljust, int len, int zpad)
+fmtnum(long_long value, int base, int dosign, int ljust, int len, int zpad,
vsnprintf_data *vsnpd)
{
int signvalue = 0;
ulong_long uvalue;
@@ -372,34 +382,34 @@
{
if (signvalue)
{
- dopr_outch(signvalue);
+ dopr_outch(signvalue, vsnpd);
--padlen;
signvalue = 0;
}
while (padlen > 0)
{
- dopr_outch(zpad);
+ dopr_outch(zpad, vsnpd);
--padlen;
}
}
while (padlen > 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
--padlen;
}
if (signvalue)
- dopr_outch(signvalue);
+ dopr_outch(signvalue, vsnpd);
while (place > 0)
- dopr_outch(convert[--place]);
+ dopr_outch(convert[--place], vsnpd);
while (padlen < 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
++padlen;
}
} static void
-fmtfloat(double value, char type, int ljust, int len, int precision, int
pointflag)
+fmtfloat(double value, char type, int ljust, int len, int precision, int
pointflag, vsnprintf_data *vsnpd)
{
char fmt[32];
char convert[512];
@@ -426,45 +436,45 @@ while (padlen > 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
--padlen;
}
- dostr(convert, 0);
+ dostr(convert, 0, vsnpd);
while (padlen < 0)
{
- dopr_outch(' ');
+ dopr_outch(' ', vsnpd);
++padlen;
}
} static void
-dostr(char *str, int cut)
+dostr(char *str, int cut, vsnprintf_data *vsnpd)
{
if (cut)
{
while (*str && cut-- > 0)
- dopr_outch(*str++);
+ dopr_outch(*str++, vsnpd);
}
else
{
while (*str)
- dopr_outch(*str++);
+ dopr_outch(*str++, vsnpd);
}
} static void
-dopr_outch(int c)
+dopr_outch(int c, vsnprintf_data *vsnpd)
{
#ifdef NOT_USED
if (iscntrl((unsigned char) c) && c != '\n' && c != '\t')
{
c = '@' + (c & 0x1F);
- if (end == 0 || output < end)
- *output++ = '^';
+ if (vsnpd->end == 0 || vsnpd->output < vsnpd->end)
+ *vsnpd->output++ = '^';
}
#endif
- if (end == 0 || output < end)
- *output++ = c;
+ if (vsnpd->end == 0 || vsnpd->output < vsnpd->end)
+ *vsnpd->output++ = c;
else
- SnprfOverflow++;
+ vsnpd->SnprfOverflow++;
}
"PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
Some OSes lack proper snprintf()/vsnprintf() fuctions so PostgreSQL includes
its own version (src/port/snprintf.c) during building. Unfortunately, this
version of snprintf() is not reentrant (it uses global vars to keep internal
state), so for example running libpq-based concurrent applications (threads)
causes libpq fuctions to fail sometimes.
What platforms have workable thread support but not snprintf? I think
this change is not likely to accomplish much except clutter the snprintf
code ...
regards, tom lane
Tom Lane wrote:
"PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
Some OSes lack proper snprintf()/vsnprintf() fuctions so PostgreSQL includes
its own version (src/port/snprintf.c) during building. Unfortunately, this
version of snprintf() is not reentrant (it uses global vars to keep internal
state), so for example running libpq-based concurrent applications (threads)
causes libpq fuctions to fail sometimes.What platforms have workable thread support but not snprintf? I think
this change is not likely to accomplish much except clutter the snprintf
code ...
What we could do is to throw a compile #error if you hit our own
snprintf() and are compiling with threads enabled.
Patch attached and applied.
--
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 19073Attachments:
/bjm/difftext/plainDownload+5-0
Bruce Momjian <pgman@candle.pha.pa.us> writes:
What we could do is to throw a compile #error if you hit our own
snprintf() and are compiling with threads enabled.
Good thought. If we get any complaints then we can reconsider applying
this patch, otherwise there's no need.
regards, tom lane
On Thu, 8 Jan 2004, Tom Lane wrote:
TL> "PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
TL> > Some OSes lack proper snprintf()/vsnprintf() fuctions so PostgreSQL includes
TL> > its own version (src/port/snprintf.c) during building. Unfortunately, this
TL> > version of snprintf() is not reentrant (it uses global vars to keep internal
TL> > state), so for example running libpq-based concurrent applications (threads)
TL> > causes libpq fuctions to fail sometimes.
TL>
TL> What platforms have workable thread support but not snprintf? I think
TL> this change is not likely to accomplish much except clutter the snprintf
TL> code ...
I discovered this problem while porting libpq (client interface) on
RTEMS OS (rtems.org). This is an embedded OS and as many other embedded OSes it
lacks non-ANSI C functions (at least RTEMS image from my vendor does not have
them). snprintf()/vsnprintf() functions are not ANSI-compliant so they should be
used with care. This OS has POSIX thread support though I did not use it (i.e. I
keep all PgSQL activity in one thread, so the code was compiled without
--enable-thread-safety). The difficulty I observed is: if even I keep PgSQL
calls serialized, calling bare snprintf() from some other thread would likely
cause concurrent PgSQL call to fail. Quite a strange result for such an
inoffensive action, don't you think so?
Anyway, I have fixed this for my code but if you think that the change is
inappropriate for the main stream then let it be. I guess you would hear some
more complaints as there will be more ports on embedded platforms.
TL>
TL> regards, tom lane
TL>
--
Thanks,
Denis.
Denis N. Stepanov wrote:
snprintf()/vsnprintf() functions are not ANSI-compliant
Yes, they are.
On Sun, 11 Jan 2004, Peter Eisentraut wrote:
PE> Denis N. Stepanov wrote:
PE> > snprintf()/vsnprintf() functions are not ANSI-compliant
PE>
PE> Yes, they are.
Sorry, I was talking about ANSI X3.159-1989, which certainly does not declare
snprintf(). In practice it is diffucult to count on, say, C99-compliant C
runtime, especially for embedded systems.
Thanks,
Denis.