Thread: 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

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