Re: [BUGS] BUG #1270: stack overflow in thread in fe_getauthname

Started by Bruce Momjianover 21 years ago8 messageshackers

bruce@momjian.us

over 21 years ago

What do people think about using (sizeof(struct passwd) + BUFLEN/2) rather
than BUFLEN for the getpwuid_r size, or (sizeof(struct passwd) + MAXPGPATH*2)?
That would reduce the stack requirements and still be safe, I think.

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

Peter Davie wrote:

Hi Bruce,

(As I explained to Tom, the situation for me is difficult:

How can I "fix the app" when the application is a commercial binary which
provides no means of configuring the stack size for the thread? I have
been successfully using PostgreSQL with this application since v7.1 and
have a happy customer with it. Now, upgrading the database to v7.4.5
which is strongly recommended by the development group (due to potentially
serious bugs) means that the application no longer works!
)

Since then I have done some background reading
<http://www.opengroup.org/onlinepubs/009695399/functions/getpwuid.html>:

[TSF] The getpwuid_r() function shall update the passwd structure pointed
to by pwd and store a pointer to that structure at the location pointed to
by result. The structure shall contain an entry from the user database
with a matching uid. Storage referenced by the structure is allocated from
the memory provided with the buffer parameter, which is bufsize bytes in
size. The maximum size needed for this buffer can be determined with the
{_SC_GETPW_R_SIZE_MAX} sysconf() parameter. A NULL pointer shall be
returned at the location pointed to by result on error or if the requested
entry is not found.

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

Thanks
Peter
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
--
Relevance Phone: +61 2 6241 6400
A.B.N. 86 063 403 690 Fax: +61 2 6241 6422
Unit 11, Mitchell Commercial Ctr, E-mail: peter.davie@relevance.com.au
cnr Brookes and Heffernan Sts., Web: http://www.relevance.com.au
Mitchell ACT 2911

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

Import Notes

Reply to msg id not found: 63143.152.91.9.144.1096335535.squirrel@152.91.9.144

Tom Lane

tgl@sss.pgh.pa.us

over 21 years ago

In reply to: Bruce Momjian (#1)

Bruce Momjian <pgman@candle.pha.pa.us> writes:

What do people think about using (sizeof(struct passwd) + BUFLEN/2) rather
than BUFLEN for the getpwuid_r size, or (sizeof(struct passwd) + MAXPGPATH*2)?
That would reduce the stack requirements and still be safe, I think.

Why bother?

Peter did not say what his closed-source app could tolerate. Without
that knowledge you're just flying blind about fixing his problem.
I see no reason to risk creating buffer-overflow issues for other people
in order to make a maybe-or-maybe-not improvement for one rather broken
closed-source app...

regards, tom lane

Peter Davie

Peter.Davie@relevance.com.au

over 21 years ago

In reply to: Tom Lane (#2)

Hi Guys,

Please refer to <http://www.opengroup.org/onlinepubs/009695399/functions/getpwuid.html>:

"[TSF] The getpwuid_r() function shall update the passwd structure pointed
to by pwd and store a pointer to that structure at the location pointed to
by result. The structure shall contain an entry from the user database
with a matching uid. Storage referenced by the structure is allocated from
the memory provided with the buffer parameter, which is bufsize bytes in
size. The maximum size needed for this buffer can be determined with the
{_SC_GETPW_R_SIZE_MAX} sysconf() parameter. A NULL pointer shall be
returned at the location pointed to by result on error or if the requested
entry is not found."

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

When I modified the source, I punted a value of 1024 and this has worked flawlessly in an intensive environment (numerous inserts per minute, sustained) for a few weeks now.

Thanks
Peter

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

What do people think about using (sizeof(struct passwd) + BUFLEN/2) rather
than BUFLEN for the getpwuid_r size, or (sizeof(struct passwd) + MAXPGPATH*2)?
That would reduce the stack requirements and still be safe, I think.

Why bother?

Peter did not say what his closed-source app could tolerate. Without
that knowledge you're just flying blind about fixing his problem.
I see no reason to risk creating buffer-overflow issues for other people
in order to make a maybe-or-maybe-not improvement for one rather broken
closed-source app...

regards, tom lane

--
Relevance... because results count

Relevance Phone: +61 (0)2 6241 6400
A.B.N. 86 063 403 690 Fax: +61 (0)2 6241 6422
Unit 11, Mitchell Commercial Centre, Mobile: +61 (0)417 265 175
cnr Brookes and Heffernan Sts., E-mail: peter.davie@relevance.com.au
Mitchell ACT 2911 Web: http://www.relevance.com.au

Bruce Momjian

bruce@momjian.us

over 21 years ago

In reply to: Peter Davie (#3)

OK, we got a report. I just thinkg 8192 is excessive for that
structure, and if someone is having a problem, others might as well.

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

Peter Davie wrote:

Hi Guys,

Please refer to <http://www.opengroup.org/onlinepubs/009695399/functions/getpwuid.html>:

"[TSF] The getpwuid_r() function shall update the passwd structure pointed
to by pwd and store a pointer to that structure at the location pointed to
by result. The structure shall contain an entry from the user database
with a matching uid. Storage referenced by the structure is allocated from
the memory provided with the buffer parameter, which is bufsize bytes in
size. The maximum size needed for this buffer can be determined with the
{_SC_GETPW_R_SIZE_MAX} sysconf() parameter. A NULL pointer shall be
returned at the location pointed to by result on error or if the requested
entry is not found."

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

When I modified the source, I punted a value of 1024 and this has worked flawlessly in an intensive environment (numerous inserts per minute, sustained) for a few weeks now.

Thanks
Peter

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

What do people think about using (sizeof(struct passwd) + BUFLEN/2) rather
than BUFLEN for the getpwuid_r size, or (sizeof(struct passwd) + MAXPGPATH*2)?
That would reduce the stack requirements and still be safe, I think.

Why bother?

Peter did not say what his closed-source app could tolerate. Without
that knowledge you're just flying blind about fixing his problem.
I see no reason to risk creating buffer-overflow issues for other people
in order to make a maybe-or-maybe-not improvement for one rather broken
closed-source app...

regards, tom lane

--
Relevance... because results count

Relevance Phone: +61 (0)2 6241 6400
A.B.N. 86 063 403 690 Fax: +61 (0)2 6241 6422
Unit 11, Mitchell Commercial Centre, Mobile: +61 (0)417 265 175
cnr Brookes and Heffernan Sts., E-mail: peter.davie@relevance.com.au
Mitchell ACT 2911 Web: http://www.relevance.com.au

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

tgl@sss.pgh.pa.us

over 21 years ago

In reply to: Bruce Momjian (#4)

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, we got a report. I just thinkg 8192 is excessive for that
structure, and if someone is having a problem, others might as well.

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

I'd be more impressed by this line of reasoning if _SC_GETPW_R_SIZE_MAX
were defined on more than one of the platforms I use...

regards, tom lane

Peter Davie

Peter.Davie@relevance.com.au

over 21 years ago

In reply to: Tom Lane (#5)

Hi Tom,

How many of these platforms you use are POSIX-compliant? This
information came from the POSIX web site (NOT THE DIGITAL/COMPAQ/HP/...
WEBSITE). Sysconf(_SC_GETPW_R_SIZE_MAX) is supported by Solaris 2.5,
SCO UNIX (circa 1999!), Digital UNIX/Compaq Tru64 UNIX, FreeBSD, AIX,
HP-UX, and probably many more.

Maybe the non-compliant platforms should be catered for as "legacy" with
support code in the "ports" area, and those that do adhere to open
standards can be accommodated without breaking existing production
applications.

Thanks
Peter

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, we got a report. I just thinkg 8192 is excessive for that
structure, and if someone is having a problem, others might as well.

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

I'd be more impressed by this line of reasoning if _SC_GETPW_R_SIZE_MAX
were defined on more than one of the platforms I use...

regards, tom lane

--
Relevance... because results count

Bruce Momjian

bruce@momjian.us

over 21 years ago

In reply to: Peter Davie (#6)

One idea would be to use malloc() to allocate storage for the
thread-safe buffers when compiled with thread-safety, rather than using
the stack.

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

Peter Davie wrote:

Hi Tom,

How many of these platforms you use are POSIX-compliant? This
information came from the POSIX web site (NOT THE DIGITAL/COMPAQ/HP/...
WEBSITE). Sysconf(_SC_GETPW_R_SIZE_MAX) is supported by Solaris 2.5,
SCO UNIX (circa 1999!), Digital UNIX/Compaq Tru64 UNIX, FreeBSD, AIX,
HP-UX, and probably many more.

Maybe the non-compliant platforms should be catered for as "legacy" with
support code in the "ports" area, and those that do adhere to open
standards can be accommodated without breaking existing production
applications.

Thanks
Peter

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, we got a report. I just thinkg 8192 is excessive for that
structure, and if someone is having a problem, others might as well.

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

I'd be more impressed by this line of reasoning if _SC_GETPW_R_SIZE_MAX
were defined on more than one of the platforms I use...

regards, tom lane

--
Relevance... because results count

Relevance Phone: +61 (0)2 6241 6400
A.B.N. 86 063 403 690 Fax: +61 (0)2 6241 6422
Unit 11, Mitchell Commercial Centre, Mobile: +61 (0)417 265 175
cnr Brookes and Heffernan Sts., E-mail: peter.davie@relevance.com.au
Mitchell ACT 2911 Web: http://www.relevance.com.au

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

Peter Davie

Peter.Davie@relevance.com.au

over 21 years ago

In reply to: Bruce Momjian (#7)

Hi Bruce,

That would work!

Thanks
Peter

Bruce Momjian wrote:

One idea would be to use malloc() to allocate storage for the
thread-safe buffers when compiled with thread-safety, rather than using
the stack.

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

Peter Davie wrote:

Hi Tom,

How many of these platforms you use are POSIX-compliant? This
information came from the POSIX web site (NOT THE DIGITAL/COMPAQ/HP/...
WEBSITE). Sysconf(_SC_GETPW_R_SIZE_MAX) is supported by Solaris 2.5,
SCO UNIX (circa 1999!), Digital UNIX/Compaq Tru64 UNIX, FreeBSD, AIX,
HP-UX, and probably many more.

Maybe the non-compliant platforms should be catered for as "legacy" with
support code in the "ports" area, and those that do adhere to open
standards can be accommodated without breaking existing production
applications.

Thanks
Peter

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, we got a report. I just thinkg 8192 is excessive for that
structure, and if someone is having a problem, others might as well.

On Tru64 UNIX, sysconf(_SC_GETPW_R_SIZE_MAX) returns 1024.

I'd be more impressed by this line of reasoning if _SC_GETPW_R_SIZE_MAX
were defined on more than one of the platforms I use...

regards, tom lane

--
Relevance... because results count

Relevance Phone: +61 (0)2 6241 6400
A.B.N. 86 063 403 690 Fax: +61 (0)2 6241 6422
Unit 11, Mitchell Commercial Centre, Mobile: +61 (0)417 265 175
cnr Brookes and Heffernan Sts., E-mail: peter.davie@relevance.com.au
Mitchell ACT 2911 Web: http://www.relevance.com.au

--
Relevance... because results count