Thread: BUG #1044: snprintf() shipped with PostgreSQL is not thread safe

BUG #1044: snprintf() shipped with PostgreSQL is not thread safe

From
"PostgreSQL Bugs List"
Date:
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++;
 }

Re: BUG #1044: snprintf() shipped with PostgreSQL is not thread safe

From
Tom Lane
Date:
"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

Re: BUG #1044: snprintf() shipped with PostgreSQL is not thread

From
Bruce Momjian
Date:
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 19073
Index: src/port/snprintf.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/port/snprintf.c,v
retrieving revision 1.2
diff -c -c -r1.2 snprintf.c
*** src/port/snprintf.c    29 Nov 2003 22:41:31 -0000    1.2
--- src/port/snprintf.c    8 Jan 2004 17:12:47 -0000
***************
*** 35,40 ****
--- 35,45 ----
  /* might be in either frontend or backend */
  #include "postgres_fe.h"

+ #ifdef ENABLE_THREAD_SAFETY
+ #error    The replacement snprintf() is not thread-safe.  \
+ Your platform must have a thread-safe snprintf() to compile with threads.
+ #endif
+
  #include <sys/ioctl.h>
  #include <sys/param.h>


Re: BUG #1044: snprintf() shipped with PostgreSQL is not thread safe

From
Tom Lane
Date:
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

Re: BUG #1044: snprintf() shipped with PostgreSQL is not

From
"Denis N. Stepanov"
Date:
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.

Re: BUG #1044: snprintf() shipped with PostgreSQL is not

From
Peter Eisentraut
Date:
Denis N. Stepanov wrote:
> snprintf()/vsnprintf() functions are not ANSI-compliant

Yes, they are.

Re: BUG #1044: snprintf() shipped with PostgreSQL is not

From
"Denis N. Stepanov"
Date:
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.