Fix unsafe coding in ResourceOwnerReleaseAll()
I wondered why the example shown in [1]/messages/by-id/19527-6e7686960c6dce78@postgresql.org 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
Attachments:
make-ResourceOwnerReleaseAll-safer.patchtext/x-diff; charset=us-ascii; name=make-ResourceOwnerReleaseAll-safer.patchDownload+15-5
Hi Tom,
Thank you for proposing this change.
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.
Another approach would be to remove the resource from the resource
owner's list directly within the ResourceRelease callbacks, just
before the resource is released.
This makes the fix specific to a resource type rather than applying it
across all resource types. This would be similar to how it's done in
ReleaseCatCacheWithOwner.
That said, the patch looks good because it makes the code consistent
with ResourceOwnerReleaseAllOfKind() and avoids the crash.
Unrelated to the patch but I noticed a typo in the ERROR message in
ResourceOwnerReleaseAllOfKind(). If we
want a check similar to ResourceOwnerForget in this function, we
should edit the ERROR message with the correct
function name.
I can propose a patch for this in a separate thread, if you'd prefer.
The error message is as follows:
void
ResourceOwnerReleaseAllOfKind(ResourceOwner owner, const ResourceOwnerDesc *kind)
{
/* Mustn't call this after we have already started releasing resources. */
if (owner->releasing)
elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name);
Thank you,
Rahila Syed
Rahila Syed <rahilasyed90@gmail.com> writes:
Unrelated to the patch but I noticed a typo in the ERROR message in
ResourceOwnerReleaseAllOfKind().
/* Mustn't call this after we have already started releasing resources. */
if (owner->releasing)
elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name);
Hmm, that definitely looks like a message that was transposed from
someplace else without much thought. It might be worth tracing the
git history to see how it got to be like that.
More: unless I'm missing something, ResourceOwnerReleaseAllOfKind
is called only from plancache.c's ReleaseAllPlanCacheRefsInOwner,
which is called only in some very random-looking ways in plpgsql.
I wonder whether there's not a bigger cleanup project indicated here.
When I posted before, I thought that ResourceOwnerReleaseAllOfKind had
a direct lineage to the old ResourceOwner code, but now I'm thinking
maybe it shouldn't exist at all. Why should plpgsql be taking special
care for particular kinds of resource entries, and why should it
suppose that it owns all instances of that kind within that resowner?
regards, tom lane