Обсуждение: BUG #15540: Use after release in ExecuteTruncateGuts

The following bug has been logged on the website:

Bug reference:      15540
Logged by:          Pan Bian
Email address:      bianpan2016@163.com
PostgreSQL version: 11.1
Operating system:   Linux
Description:

File: src/backend/commands/tablecmds.c
Function: ExecuteTruncateGuts
Issue details:
The function ExecuteTruncateGuts drops the reference to rel via
relation_close when toast_relid is valid. However, after that, rel is passed
to pgstat_count_truncate. This may result in a use-after-release bug. Maybe,
rel should be re-declared on the branch that toast_relid is valid.

For your convenience, I copy-and-paste related code as follows:

void
ExecuteTruncateGuts(List *explicit_rels, List *relids, List
*relids_logged,
                    DropBehavior behavior, bool restart_seqs)
{
    ...
    foreach(cell, rels)
    {
        Relation    rel = (Relation) lfirst(cell);
        ...
        if (rel->rd_createSubid == mySubid ||
            rel->rd_newRelfilenodeSubid == mySubid)
        {
            /* Immediate, non-rollbackable truncation is OK */
            heap_truncate_one_rel(rel);
        }
        else
        {
            Oid         heap_relid;
            Oid         toast_relid;
            ...
            toast_relid = rel->rd_rel->reltoastrelid;

            /*
             * The same for the toast table, if any.
             */
            if (OidIsValid(toast_relid))
            {
                rel = relation_open(toast_relid, AccessExclusiveLock);
//### open a relation
                RelationSetNewRelfilenode(rel,
rel->rd_rel->relpersistence,
                                          RecentXmin, minmulti);
                if (rel->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED)
                    heap_create_init_fork(rel);
                heap_close(rel, NoLock);   //### release the relation
            }

            /*
             * Reconstruct the indexes to match, and we're done.
             */
            reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST, 0);
        }

        pgstat_count_truncate(rel);   //### the released relation is used
again
    }
}

Thank you,
Pan Bian

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> The function ExecuteTruncateGuts drops the reference to rel via
> relation_close when toast_relid is valid. However, after that, rel is passed
> to pgstat_count_truncate. This may result in a use-after-release bug.

... and, even more to the point, the truncation stats count is incorrectly
applied to the toast table not its parent.

> Maybe,
> rel should be re-declared on the branch that toast_relid is valid.

Yeah, seems like the right way.  Will fix.

Are you using a static analyzer to find these?  I'm curious how
you noticed them.

            regards, tom lane

On 2018-Dec-07, Tom Lane wrote:

> =?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> > The function ExecuteTruncateGuts drops the reference to rel via
> > relation_close when toast_relid is valid. However, after that, rel is passed
> > to pgstat_count_truncate. This may result in a use-after-release bug.
> 
> ... and, even more to the point, the truncation stats count is incorrectly
> applied to the toast table not its parent.

Whoops!  Thanks for fixing.  You're right, we quite likely didn't
specifically test the case of there being a toast table.  Amazing that
it took so long to find it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 07, 2018 at 11:09:05AM -0500, Tom Lane wrote:
> =?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> > The function ExecuteTruncateGuts drops the reference to rel via
> > relation_close when toast_relid is valid. However, after that, rel is passed
> > to pgstat_count_truncate. This may result in a use-after-release bug.
> 
> ... and, even more to the point, the truncation stats count is incorrectly
> applied to the toast table not its parent.
> 
> > Maybe,
> > rel should be re-declared on the branch that toast_relid is valid.
> 
> Yeah, seems like the right way.  Will fix.
> 
> Are you using a static analyzer to find these?  I'm curious how
> you noticed them.

Yes. I write a static analysis tool. It can find functions that release
memory or other resources. Let's call them free-like functions. With such
free-like functions, the tool then performs data flow analysis to find 
use-after-free bugs. Of course, we can feed those free-like functions to
other static analyzers such as Coverity. I believe it will work too.

Best regards,
Pan Bian

> 
>             regards, tom lane

On Sun, Dec 09, 2018 at 08:56:17AM +0800, PanBian wrote:
> Yes. I write a static analysis tool. It can find functions that release
> memory or other resources. Let's call them free-like functions. With such
> free-like functions, the tool then performs data flow analysis to find
> use-after-free bugs. Of course, we can feed those free-like functions to
> other static analyzers such as Coverity. I believe it will work too.

Interesting.  Did you release this stuff in the open?  I could be very
interesting to get that plugged in more easily with Postgres.  Community
runs Coverity as well.  The reports are not public still if that helps
in reporting real issues and not only false positives that would be
nice.
--
Michael