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

*TextDatumGetCString* calls palloc to alloc memory for the option
text datum, in some cases the the memory is allocated in
*TopTransactionContext*, this may cause memory leak for a long
running backend.
---
 src/backend/access/common/reloptions.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index 609329bb21..6076677aef 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1360,6 +1360,7 @@ untransformRelOptions(Datum options)
  val = (Node *) makeString(pstrdup(p));
  }
  result = lappend(result, makeDefElem(pstrdup(s), val, -1));
+ pfree(s);
  }

  return result;
-- 
2.33.0

-- 
Regards
Junwang Zhao

> On 1 Sep 2022, at 10:36, Junwang Zhao <zhjwpku@gmail.com> wrote:

> *TextDatumGetCString* calls palloc to alloc memory for the option
> text datum, in some cases the the memory is allocated in
> *TopTransactionContext*, this may cause memory leak for a long
> running backend.

Wouldn't that be a fairly small/contained leak in comparison to memory spent
during a long running transaction?  Do you have any example of transforming
reloptions in a loop into TopTransactionContext where it might add up?

--
Daniel Gustafsson        https://vmware.com/

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.

            regards, tom lane

On Thu, Sep 1, 2022 at 10:10 PM Tom Lane <tgl@sss.pgh.pa.us> 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.
>
Maybe it's pstrdup'ing s so that the caller should take care of the free?

I'm a little confused when we should call *pfree* and when we should not.
A few lines before there is a call *text_to_cstring* in which it invokes
*pfree* to free the unpacked text [0]. I'm just thinking that since *s* has
been duplicated, we should free it, that's where the patch comes from.

[0]:
```
char *
text_to_cstring(const text *t)
{
    /* must cast away the const, unfortunately */
    text *tunpacked = pg_detoast_datum_packed(unconstify(text *, t));
    int len = VARSIZE_ANY_EXHDR(tunpacked);
    char *result;

    result = (char *) palloc(len + 1);
    memcpy(result, VARDATA_ANY(tunpacked), len);
    result[len] = '\0';

    if (tunpacked != t)
        pfree(tunpacked);

    return result;
}
```

>                         regards, tom lane



-- 
Regards
Junwang Zhao

Junwang Zhao <zhjwpku@gmail.com> writes:
> I'm a little confused when we should call *pfree* and when we should not.
> A few lines before there is a call *text_to_cstring* in which it invokes
> *pfree* to free the unpacked text [0]. I'm just thinking that since *s* has
> been duplicated, we should free it, that's where the patch comes from.

By and large, the server is designed so that small memory leaks don't
matter: the space will be reclaimed when the current memory context
is deleted, and most code runs in reasonably short-lived contexts.
Individually pfree'ing such allocations is actually a net negative,
because it costs cycles and code space.

There are places where a leak *does* matter, but unless you can
demonstrate that this is one, it's not worth changing.

            regards, tom lane

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/

On 2022-Sep-09, Alvaro Herrera wrote:

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

Pushed.  This is pretty much cosmetic, so no backpatch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)