[PATCH] avoid buffer underflow in errfinish()

Started by Xi Wangabout 13 years ago8 messageshackers

xi.wang@gmail.com

about 13 years ago

CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.
---
src/backend/utils/error/elog.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 3a211bf..47a0a8b 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -393,13 +393,15 @@ void
 errfinish(int dummy,...)
 {
 	ErrorData  *edata = &errordata[errordata_stack_depth];
-	int			elevel = edata->elevel;
+	int			elevel;
 	MemoryContext oldcontext;
 	ErrorContextCallback *econtext;

recursion_depth++;
CHECK_STACK_DEPTH();

+ elevel = edata->elevel;
+
/*
* Do processing in ErrorContext, which we hope has enough reserved space
* to report an error.
--
1.7.10.4

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Xi Wang

xi.wang@gmail.com

about 13 years ago

In reply to: Xi Wang (#1)

Re: [PATCH] avoid buffer underflow in errfinish()

A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
null pointer deference for `autovac'?

There is a null pointer check `autovac != NULL', but the pointer is
already dereferenced earlier when initializing `autovac_pgxact'. Is
this null pointer check redundant, or should we move the dereference
`autovac->pgprocno' after the check? Thanks.

On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang <xi.wang@gmail.com> wrote:

CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.
---
src/backend/utils/error/elog.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 3a211bf..47a0a8b 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -393,13 +393,15 @@ void
errfinish(int dummy,...)
{
ErrorData  *edata = &errordata[errordata_stack_depth];
-       int                     elevel = edata->elevel;
+       int                     elevel;
MemoryContext oldcontext;
ErrorContextCallback *econtext;
recursion_depth++;
CHECK_STACK_DEPTH();

+ elevel = edata->elevel;
+
/*
* Do processing in ErrorContext, which we hope has enough reserved space
* to report an error.
--
1.7.10.4

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

about 13 years ago

In reply to: Xi Wang (#1)

Re: [PATCH] avoid buffer underflow in errfinish()

On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang <xi.wang@gmail.com> wrote:

CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.

This seems sensible and I'm inclined to commit it. It's unlikely to
matter very much in practice, since the only point of checking the
stack depth in the first place is to catch a seemingly-unlikely coding
error; and it's unlikely that referencing beyond the stack bounds
would do anything too horrible, either. But we may as well do it
right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

about 13 years ago

In reply to: Xi Wang (#2)

Re: [PATCH] avoid buffer underflow in errfinish()

On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang <xi.wang@gmail.com> wrote:

A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
null pointer deference for `autovac'?

Not really. If the deadlock_state is DS_BLOCKED_BY_AUTOVACUUM, there
has to be a blocking autovacuum proc. As in the other case that you
found, though, some code rearrangement would likely make the intent of
the code more clear and avoid future mistakes.

Perhaps something like:

if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM &&
allow_autovacuum_cancel
&& (autovac = GetBlockingAutoVacuumPgproc()) != NULL)
{
PGXACT *autovac_pgxact =
&ProcGlobal->allPgXact[autovac->pgprocno];
...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Heikki Linnakangas

heikki.linnakangas@enterprisedb.com

about 13 years ago

In reply to: Robert Haas (#4)

Re: [PATCH] avoid buffer underflow in errfinish()

On 27.03.2013 14:50, Robert Haas wrote:

On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang<xi.wang@gmail.com> wrote:

A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
null pointer deference for `autovac'?

I think you mean on line 1142:

PGPROC *autovac = GetBlockingAutoVacuumPgproc();
*HERE* PGXACT *autovac_pgxact = &ProcGlobal->allPgXact[autovac->pgprocno];

LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);

/*
* Only do it if the worker is not working to protect against Xid
* wraparound.
*/
if ((autovac != NULL) &&
(autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
!(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))

Not really. If the deadlock_state is DS_BLOCKED_BY_AUTOVACUUM, there
has to be a blocking autovacuum proc. As in the other case that you
found, though, some code rearrangement would likely make the intent of
the code more clear and avoid future mistakes.

Perhaps something like:

if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM&&
allow_autovacuum_cancel
&& (autovac = GetBlockingAutoVacuumPgproc()) != NULL)
{
PGXACT *autovac_pgxact =
&ProcGlobal->allPgXact[autovac->pgprocno];
...

Writing it like that suggests that autovac might sometimes be NULL, even
if deadlock_state == DS_BLOCKED_BY_AUTOVACUUM. From your explanation
above, I gather that's not possible (and I think you're right), so the
NULL check is unnecessary. If we think it might be NULL after all, the
above makes sense.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Xi Wang

xi.wang@gmail.com

about 13 years ago

In reply to: Heikki Linnakangas (#5)

Re: [PATCH] avoid buffer underflow in errfinish()

On Wed, Mar 27, 2013 at 9:03 AM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Writing it like that suggests that autovac might sometimes be NULL, even if
deadlock_state == DS_BLOCKED_BY_AUTOVACUUM. From your explanation above, I
gather that's not possible (and I think you're right), so the NULL check is
unnecessary. If we think it might be NULL after all, the above makes sense.

That makes sense. Thanks for the clarification!

- xi

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Bruce Momjian

bruce@momjian.us

over 12 years ago

In reply to: Robert Haas (#3)

Re: [PATCH] avoid buffer underflow in errfinish()

On Wed, Mar 27, 2013 at 08:45:51AM -0400, Robert Haas wrote:

On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang <xi.wang@gmail.com> wrote:

CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.

This seems sensible and I'm inclined to commit it. It's unlikely to
matter very much in practice, since the only point of checking the
stack depth in the first place is to catch a seemingly-unlikely coding
error; and it's unlikely that referencing beyond the stack bounds
would do anything too horrible, either. But we may as well do it
right.

Was this ever dealt with?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 12 years ago

In reply to: Bruce Momjian (#7)

Re: [PATCH] avoid buffer underflow in errfinish()

On Sat, Nov 30, 2013 at 2:00 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Mar 27, 2013 at 08:45:51AM -0400, Robert Haas wrote:

On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang <xi.wang@gmail.com> wrote:

CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.

This seems sensible and I'm inclined to commit it. It's unlikely to
matter very much in practice, since the only point of checking the
stack depth in the first place is to catch a seemingly-unlikely coding
error; and it's unlikely that referencing beyond the stack bounds
would do anything too horrible, either. But we may as well do it
right.

Was this ever dealt with?

No, it fell through the cracks. I have just committed it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers