Re: [PATCH v1] fix potential memory leak in untransformRelOptions

On 2022-Sep-01, Tom Lane wrote:

> Junwang Zhao <zhjwpku(at)gmail(dot)com> writes:
> > result = lappend(result, makeDefElem(pstrdup(s), val, -1));
> > + pfree(s);
>
> I wonder why it's pstrdup'ing s in the first place.

Yeah, I think both the pstrdups in that function are useless. The
DefElems can just point to the correct portion of the (already pstrdup'd
by TextDatumGetCString) copy of optiondatums[i]. We modify that copy to
install \0 in the place where the = is, and that copy is not freed
anywhere.

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 609329bb21..0aa4b334ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1357,9 +1357,9 @@ untransformRelOptions(Datum options)
if (p)
{
*p++ = '\0';
- val = (Node *) makeString(pstrdup(p));
+ val = (Node *) makeString(p);
}
- result = lappend(result, makeDefElem(pstrdup(s), val, -1));
+ result = lappend(result, makeDefElem(s, val, -1));
}

return result;

I think these pstrdups were already not necessary when the function was
added in 265f904d8f25, because textout() was already known to return a
palloc'ed copy of its input; but later 220db7ccd8c8 made this contract
even more explicit.

Keeping 's' and removing the pstrdups better uses memory, because we
have a single palloc'ed chunk per option rather than two.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/