Issue with some calls to GetMultiXactIdMembers()
Hi,
There's a couple of calls to GetMultiXactIdMembers() in heapam.c which
subsequently pfree() the returned "members" pointer (pass-by-reference
parameter) if it's non-NULL.
However, there's an error return within GetMultiXactIdMembers() that
returns -1 without NULLing out "members", and the callers have simply
allocated that pointer on the stack without initializing it to NULL.
If that error condition were to ever happen, pfree() would likely be
called with a junk value.
Also note that there's another error return (about 15 lines further
down) in GetMultiXactIdMembers() that returns -1 and does NULL out
"members", so the handling is inconsistent.
The attached patch adds the NULLing out of the "members" pointer in
the first error case, to fix that and guard against possible pfree()
on error by such callers.
I also note that there are other callers which pfree() "members" based
on the returned "nmembers" value, and this is also inconsistent.
Some pfree() "members" if nmembers>= 0, while others pfree() it if nmembers>0.
After looking at the code for a while, it looks like the "nmembers ==
0" case can't actually happen (right?). I decided not to mess with any
of the calling code.
Regards,
Greg Nancarrow
Fujitsu Australia
Attachments:
0001-Fix-possible-pfree-of-junk-members-value.patchapplication/octet-stream; name=0001-Fix-possible-pfree-of-junk-members-value.patchDownload
From 69463b95e54fd5c179845b418c5a25afbee663c4 Mon Sep 17 00:00:00 2001
From: Greg Nancarrow <gregn4422@gmail.com>
Date: Wed, 16 Jun 2021 18:58:23 +1000
Subject: [PATCH] Fix potential case of attempted pfree() of junk "members"
value on GetMultiXactIdMembers() error.
---
src/backend/access/transam/multixact.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index daab546f29..a81fe72d3c 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1241,7 +1241,10 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
if (!MultiXactIdIsValid(multi) || from_pgupgrade)
+ {
+ *members = NULL;
return -1;
+ }
/* See if the MultiXactId is in the local cache */
length = mXactCacheGetById(multi, members);
--
2.27.0
On 16/06/2021 13:22, Greg Nancarrow wrote:
Hi,
There's a couple of calls to GetMultiXactIdMembers() in heapam.c which
subsequently pfree() the returned "members" pointer (pass-by-reference
parameter) if it's non-NULL.
However, there's an error return within GetMultiXactIdMembers() that
returns -1 without NULLing out "members", and the callers have simply
allocated that pointer on the stack without initializing it to NULL.
If that error condition were to ever happen, pfree() would likely be
called with a junk value.
Also note that there's another error return (about 15 lines further
down) in GetMultiXactIdMembers() that returns -1 and does NULL out
"members", so the handling is inconsistent.
The attached patch adds the NULLing out of the "members" pointer in
the first error case, to fix that and guard against possible pfree()
on error by such callers.
Thanks! Committed with a few additional cleanups.
I also note that there are other callers which pfree() "members" based
on the returned "nmembers" value, and this is also inconsistent.
Some pfree() "members" if nmembers>= 0, while others pfree() it if nmembers>0.
After looking at the code for a while, it looks like the "nmembers ==
0" case can't actually happen (right?). I decided not to mess with any
of the calling code.
I added an assertion that it never returns nmembers==0.
- Heikki