Thread: BUG #1270: stack overflow in thread in fe_getauthname

The following bug has been logged online:

Bug reference:      1270
Logged by:          Peter Davie

Email address:      Peter.Davie@relevance.com.au

PostgreSQL version: 7.4.5

Operating system:   OSF/1 4.0f

Description:        stack overflow in thread in fe_getauthname

Details:

With the THREAD_SAFETY changes, a buffer is defined on the stack as:
char       pwdbuf[BUFSIZ];

This buffer overflows the stack when used in a thread.  As the application
creating the thread cannot be modified to increase the stack size, it would
probably be prudent to reduce this buffer size (I believe that BUFSIZ is
around 8192 bytes on most modern Unix implementations).

To rectify this issue (seg faults attempting to connect to the database), I
replaced the above declaration with:
char       pwdbuf[1024];
Obviously, a manifest constant would be better!

Oops.  Yep, that is sloppy programming on our part, perhaps my part if I
added those.  Anyway, patch attached and applied. I used the proper
struct sizes instead of BUFSIZ.

This will be in 8.0.  I think it is too risky for 7.4.X but if others
disagree, let me know.

---------------------------------------------------------------------------

PostgreSQL Bugs List wrote:
>
> The following bug has been logged online:
>
> Bug reference:      1270
> Logged by:          Peter Davie
>
> Email address:      Peter.Davie@relevance.com.au
>
> PostgreSQL version: 7.4.5
>
> Operating system:   OSF/1 4.0f
>
> Description:        stack overflow in thread in fe_getauthname
>
> Details:
>
> With the THREAD_SAFETY changes, a buffer is defined on the stack as:
> char       pwdbuf[BUFSIZ];
>
> This buffer overflows the stack when used in a thread.  As the application
> creating the thread cannot be modified to increase the stack size, it would
> probably be prudent to reduce this buffer size (I believe that BUFSIZ is
> around 8192 bytes on most modern Unix implementations).
>
> To rectify this issue (seg faults attempting to connect to the database), I
> replaced the above declaration with:
> char       pwdbuf[1024];
> Obviously, a manifest constant would be better!
>
>
> ---------------------------(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
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.91
diff -c -c -r1.91 fe-auth.c
*** src/interfaces/libpq/fe-auth.c    29 Aug 2004 04:13:12 -0000    1.91
--- src/interfaces/libpq/fe-auth.c    27 Sep 2004 23:34:55 -0000
***************
*** 749,755 ****
          if (GetUserName(username, &namesize))
              name = username;
  #else
!         char        pwdbuf[BUFSIZ];
          struct passwd pwdstr;
          struct passwd *pw = NULL;

--- 749,755 ----
          if (GetUserName(username, &namesize))
              name = username;
  #else
!         char        pwdbuf[sizeof(struct passwd)];
          struct passwd pwdstr;
          struct passwd *pw = NULL;

Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.52
diff -c -c -r1.52 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    26 Sep 2004 22:51:49 -0000    1.52
--- src/interfaces/libpq/fe-secure.c    27 Sep 2004 23:34:56 -0000
***************
*** 512,518 ****

      {
          struct hostent hpstr;
!         char        buf[BUFSIZ];
          int            herrno = 0;

          /*
--- 512,518 ----

      {
          struct hostent hpstr;
!         char        buf[sizeof(struct hostent)];
          int            herrno = 0;

          /*
***************
*** 598,604 ****
  #ifdef WIN32
      return NULL;
  #else
!     char        pwdbuf[BUFSIZ];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      FILE       *fp;
--- 598,604 ----
  #ifdef WIN32
      return NULL;
  #else
!     char        pwdbuf[sizeof(struct passwd)];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      FILE       *fp;
***************
*** 745,751 ****
  #ifdef WIN32
      return 0;
  #else
!     char        pwdbuf[BUFSIZ];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      struct stat buf,
--- 745,751 ----
  #ifdef WIN32
      return 0;
  #else
!     char        pwdbuf[sizeof(struct passwd)];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      struct stat buf,
***************
*** 952,958 ****
  {
  #ifndef WIN32
      struct stat buf;
!     char        pwdbuf[BUFSIZ];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      char        fnbuf[MAXPGPATH];
--- 952,958 ----
  {
  #ifndef WIN32
      struct stat buf;
!     char        pwdbuf[sizeof(struct passwd)];
      struct passwd pwdstr;
      struct passwd *pwd = NULL;
      char        fnbuf[MAXPGPATH];
Index: src/port/getaddrinfo.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/port/getaddrinfo.c,v
retrieving revision 1.13
diff -c -c -r1.13 getaddrinfo.c
*** src/port/getaddrinfo.c    27 Sep 2004 23:24:45 -0000    1.13
--- src/port/getaddrinfo.c    27 Sep 2004 23:34:57 -0000
***************
*** 85,91 ****

  #ifdef FRONTEND
              struct hostent hpstr;
!             char        buf[BUFSIZ];
              int            herrno = 0;

              pqGethostbyname(node, &hpstr, buf, sizeof(buf),
--- 85,91 ----

  #ifdef FRONTEND
              struct hostent hpstr;
!             char        buf[sizeof(struct hostent)];
              int            herrno = 0;

              pqGethostbyname(node, &hpstr, buf, sizeof(buf),
Index: src/port/thread.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/port/thread.c,v
retrieving revision 1.26
diff -c -c -r1.26 thread.c
*** src/port/thread.c    27 Sep 2004 23:24:45 -0000    1.26
--- src/port/thread.c    27 Sep 2004 23:34:58 -0000
***************
*** 103,109 ****
      /* POSIX version */
      getpwuid_r(uid, resultbuf, buffer, buflen, result);
  #else
-
      /*
       * Early POSIX draft of getpwuid_r() returns 'struct passwd *'.
       * getpwuid_r(uid, resultbuf, buffer, buflen)
--- 103,108 ----
***************
*** 111,117 ****
      *result = getpwuid_r(uid, resultbuf, buffer, buflen);
  #endif
  #else
-
      /* no getpwuid_r() available, just use getpwuid() */
      *result = getpwuid(uid);
  #endif
--- 110,115 ----

"PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
> With the THREAD_SAFETY changes, a buffer is defined on the stack as:
> char       pwdbuf[BUFSIZ];
> This buffer overflows the stack when used in a thread.  As the application
> creating the thread cannot be modified to increase the stack size, it would
> probably be prudent to reduce this buffer size (I believe that BUFSIZ is
> around 8192 bytes on most modern Unix implementations).

No, it would be prudent to fix the app.  While this one particular
buffer might be larger than needed, we are *not* going to buy into the
notion that libpq needs to run successfully in an 8K stack.  This
particular problem is only the tip of the iceberg; hewing to any
such limit is going to require far more drastic changes that just don't
seem worthwhile.

            regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Oops.  Yep, that is sloppy programming on our part, perhaps my part if I
> added those.  Anyway, patch attached and applied. I used the proper
> struct sizes instead of BUFSIZ.

You just broke it.

Those buffers are not used to hold struct passwd's, but to hold
multiple character strings to which the struct passwd will point;
any one of which could be long, but particularly the home directory
path.

My man page for getpwuid_r says that the minimum recommended buffer size
is 1024.

> This will be in 8.0.

I think we should revert it entirely.  A small buffer size risks
breaking things unnecessarily, and as I replied earlier, the request
to make libpq run in a less-than-8K stack is not reasonable anyway.

            regards, tom lane

Tom Lane wrote:
> "PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
> > With the THREAD_SAFETY changes, a buffer is defined on the stack as:
> > char       pwdbuf[BUFSIZ];
> > This buffer overflows the stack when used in a thread.  As the application
> > creating the thread cannot be modified to increase the stack size, it would
> > probably be prudent to reduce this buffer size (I believe that BUFSIZ is
> > around 8192 bytes on most modern Unix implementations).
>
> No, it would be prudent to fix the app.  While this one particular
> buffer might be larger than needed, we are *not* going to buy into the
> notion that libpq needs to run successfully in an 8K stack.  This
> particular problem is only the tip of the iceberg; hewing to any
> such limit is going to require far more drastic changes that just don't
> seem worthwhile.

Agreed.  I fixed the cases were the buffers was really too large, but
there is no way we could run on an 8k stack.  I assume you were having
problems where you were doing multiple lookups in a single thread but am
not sure that ever really happens.

--
  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

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Oops.  Yep, that is sloppy programming on our part, perhaps my part if I
> > added those.  Anyway, patch attached and applied. I used the proper
> > struct sizes instead of BUFSIZ.
>
> You just broke it.
>
> Those buffers are not used to hold struct passwd's, but to hold
> multiple character strings to which the struct passwd will point;
> any one of which could be long, but particularly the home directory
> path.
>
> My man page for getpwuid_r says that the minimum recommended buffer size
> is 1024.
>
> > This will be in 8.0.
>
> I think we should revert it entirely.  A small buffer size risks
> breaking things unnecessarily, and as I replied earlier, the request
> to make libpq run in a less-than-8K stack is not reasonable anyway.

Reverted.  I forgot about the requirement to store pointers used by the
structure.  I knew that when doing the thread patches but forgot about
it.

--
  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