[PATCH v1] fix potential memory leak in untransformRelOptions

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]``` 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;. 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;
```
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

got it, thanks.

Tom Lane <tgl@sss.pgh.pa.us>于2022年9月2日 周五01:13写道：

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

--
Regards
Junwang Zhao

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)