Re: [PATCH v1] fix potential memory leak in untransformRelOptions - Mailing list pgsql-hackers

On 2022-Sep-01, Tom Lane wrote:

> Junwang Zhao <zhjwpku@gmail.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/