Fix unsafe order of operations in ResourceOwnerReleaseAll().

This function called the resource-kind-specific ReleaseResource()
method for each item before deleting that item from the resowner.
That's backwards from the ordering in ResourceOwnerReleaseAllOfKind,
and it's not very safe.  If ReleaseResource throws an error then the
subsequent abort cleanup will come back here and try to release that
item again, possibly leading to a double-free or similar crash,
and in any case risking an infinite error cleanup loop.  This mistake
explains why the pgcrypto bug just fixed in 80bb0ebcc led to a crash
rather than something more benign.

Remove the item from the resowner, then call ReleaseResource,
matching the way things were done before b8bff07da.  If there
is a problem of this sort, we'd prefer to leak the item than
suffer the other likely consequences.

Per further analysis of bug #19527.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/646741.1782157515@sss.pgh.pa.us
Backpatch-through: 17

diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index 06e1121c5ffdd6226a15fb7d4ae107e0b880dd09..03d2f5a0334bfdd3edf0f08c4f513f7a3a8fe137 100644 (file)
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -347,6 +347,7 @@ ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase,
 {
        ResourceElem *items;
        uint32          nitems;
+       bool            using_arr;
 
        /*
         * ResourceOwnerSort must've been called already.  All the resources are
@@ -358,12 +359,14 @@ ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase,
        {
                items = owner->arr;
                nitems = owner->narr;
+               using_arr = true;
        }
        else
        {
                Assert(owner->narr == 0);
                items = owner->hash;
                nitems = owner->nhash;
+               using_arr = false;
        }
 
        /*
@@ -392,13 +395,20 @@ ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase,
                        elog(WARNING, "resource was not closed: %s", res_str);
                        pfree(res_str);
                }
-               kind->ReleaseResource(value);
+
+               /*
+                * Update stored count to forget the item before calling its
+                * ReleaseResource method.  This avoids double-free crashes in case an
+                * error gets thrown within ReleaseResource.
+                */
                nitems--;
+               if (using_arr)
+                       owner->narr = nitems;
+               else
+                       owner->nhash = nitems;
+
+               kind->ReleaseResource(value);
        }
-       if (owner->nhash == 0)
-               owner->narr = nitems;
-       else
-               owner->nhash = nitems;
 }