Fix unsafe coding in ResourceOwnerReleaseAll()

[Tom Lane]

I wondered why the example shown in [1] led to a double-free crash
rather than a clean failure.  On investigation, the problem is that
ResourceOwnerReleaseAll() calls the resource kind's ReleaseResource
method before deleting the resource from the owner, rather than
afterwards as the code in ResourceOwnerReleaseAllOfKind() does.
So if we throw an error inside ReleaseResource, the subsequent abort
cleanup comes back and does resource owner cleanup again, and we
try to delete the item again.  Kaboom.

The attached patch converts the bug example into a clean failure,
even with the recent bug fix in pgcrypto undone:

regression=# SELECT encrypt_iv(
    repeat('A', 1073741308)::bytea,
    decode('00112233445566778899aabbccddeeff', 'hex'),
    decode('000102030405060708090a0b0c0d0e0f', 'hex'),
    'aes'
);
ERROR:  invalid memory alloc request size 1073741824
WARNING:  AbortTransaction while in ABORT state
ERROR:  ResourceOwnerForget called for pgcrypto OpenSSL cipher handle after 
release started
regression=# 

Of course, this might lead to leaking the resource we wished to free.
But that's better than crashing, or at least that's the value judgment
we made long ago in the original ResourceOwner code.

                        regards, tom lane

[1] 
https://www.postgresql.org/message-id/19527-6e7686960c6dce78%40postgresql.org


diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index 06e1121c5ff..03d2f5a0334 100644
--- 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;
 }