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

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

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(dot)wang(at)gmail(dot)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
>

On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang <xi(dot)wang(at)gmail(dot)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

On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang <xi(dot)wang(at)gmail(dot)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

On 27.03.2013 14:50, Robert Haas wrote:
> On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang<xi(dot)wang(at)gmail(dot)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

On Wed, Mar 27, 2013 at 9:03 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)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

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(dot)wang(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Sat, Nov 30, 2013 at 2:00 PM, Bruce Momjian <bruce(at)momjian(dot)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(dot)wang(at)gmail(dot)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