public inbox for [email protected]
help / color / mirror / Atom feedAvoid resource leak (contrib/postgres_fdw/connection.c)
5+ messages / 3 participants
[nested] [flat]
* Avoid resource leak (contrib/postgres_fdw/connection.c)
@ 2026-03-16 11:45 Ranier Vilela <[email protected]>
2026-03-16 14:16 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Matheus Alcantara <[email protected]>
2026-03-16 23:07 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Fujii Masao <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Ranier Vilela @ 2026-03-16 11:45 UTC (permalink / raw)
To: pgsql-hackers
Hi.
Per Coverity.
CID 1645716: (#1 of 1): Resource leak (RESOURCE_LEAK)
8. leaked_storage: Variable str going out of scope leaks the storage
str.data points to.
The function *postgres_fdw_connection* leaks the contents of
var str.data
Once that function *cstring_to_text* palloc the contents
must be necessary to free the var str.data.
patch attached.
best regards,
Ranier Vilelas
Attachments:
[application/octet-stream] avoid-resource-leak-postgres_fdw-connection.patch (827B, 3-avoid-resource-leak-postgres_fdw-connection.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7e2b822d16..87f3e3727f 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -2355,6 +2355,7 @@ postgres_fdw_connection(PG_FUNCTION_ARGS)
ForeignServer *server = GetForeignServer(serverid);
UserMapping *user = GetUserMapping(userid, serverid);
StringInfoData str;
+ text *result;
const char **keywords;
const char **values;
char *appname;
@@ -2371,12 +2372,15 @@ postgres_fdw_connection(PG_FUNCTION_ARGS)
appendEscapedValue(&str, values[i]);
sep = " ";
}
+ result = cstring_to_text(str.data);
if (appname != NULL)
pfree(appname);
pfree(keywords);
pfree(values);
- PG_RETURN_TEXT_P(cstring_to_text(str.data));
+ pfree(str.data);
+
+ PG_RETURN_TEXT_P(result);
}
/*
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Avoid resource leak (contrib/postgres_fdw/connection.c)
2026-03-16 11:45 Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
@ 2026-03-16 14:16 ` Matheus Alcantara <[email protected]>
2026-03-16 14:18 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Matheus Alcantara @ 2026-03-16 14:16 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; pgsql-hackers
On 16/03/26 08:45, Ranier Vilela wrote:
> Hi.
>
> Per Coverity.
>
> CID 1645716: (#1 of 1): Resource leak (RESOURCE_LEAK)
> 8. leaked_storage: Variable str going out of scope leaks the storage
> str.data points to.
>
> The function *postgres_fdw_connection* leaks the contents of
> var str.data
> Once that function *cstring_to_text* palloc the contents
> must be necessary to free the var str.data.
>
> patch attached.
>
Hi,
Thanks for the patch, it looks correct to me. I've searched for this
pattern `PG_RETURN_TEXT_P(cstring_to_text(.*.data` in other places and
I've just found on postgres_fdw/connection.c
I've also search for other cases of `return cstring_to_text(...)`
usages and I didn't found anything that seems suspicious.
Tests are also passing.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Avoid resource leak (contrib/postgres_fdw/connection.c)
2026-03-16 11:45 Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
2026-03-16 14:16 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Matheus Alcantara <[email protected]>
@ 2026-03-16 14:18 ` Ranier Vilela <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Ranier Vilela @ 2026-03-16 14:18 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: pgsql-hackers
Hi.
Em seg., 16 de mar. de 2026 às 11:16, Matheus Alcantara <
[email protected]> escreveu:
> On 16/03/26 08:45, Ranier Vilela wrote:
> > Hi.
> >
> > Per Coverity.
> >
> > CID 1645716: (#1 of 1): Resource leak (RESOURCE_LEAK)
> > 8. leaked_storage: Variable str going out of scope leaks the storage
> > str.data points to.
> >
> > The function *postgres_fdw_connection* leaks the contents of
> > var str.data
> > Once that function *cstring_to_text* palloc the contents
> > must be necessary to free the var str.data.
> >
> > patch attached.
> >
>
> Hi,
>
> Thanks for the patch, it looks correct to me. I've searched for this
> pattern `PG_RETURN_TEXT_P(cstring_to_text(.*.data` in other places and
> I've just found on postgres_fdw/connection.c
>
> I've also search for other cases of `return cstring_to_text(...)`
> usages and I didn't found anything that seems suspicious.
>
> Tests are also passing.
>
Thanks for taking a look.
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Avoid resource leak (contrib/postgres_fdw/connection.c)
2026-03-16 11:45 Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
@ 2026-03-16 23:07 ` Fujii Masao <[email protected]>
2026-03-18 17:42 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Matheus Alcantara <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Fujii Masao @ 2026-03-16 23:07 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: pgsql-hackers
On Mon, Mar 16, 2026 at 8:46 PM Ranier Vilela <[email protected]> wrote:
>
> Hi.
>
> Per Coverity.
>
> CID 1645716: (#1 of 1): Resource leak (RESOURCE_LEAK)
> 8. leaked_storage: Variable str going out of scope leaks the storage str.data points to.
>
> The function *postgres_fdw_connection* leaks the contents of
> var str.data
> Once that function *cstring_to_text* palloc the contents
> must be necessary to free the var str.data.
It seems that postgres_fdw_connection() is expected to be called by
ForeignServerConnectionString() via OidFunctionCall3(),
and to allocate memory (including str.data) in the FDWConnectionContext
memory context created by ForeignServerConnectionString().
After calling postgres_fdw_connection(), ForeignServerConnectionString()
deletes FDWConnectionContext. So it seems to me that any memory
allocated in that context, including str.data, would not leak. No?
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Avoid resource leak (contrib/postgres_fdw/connection.c)
2026-03-16 11:45 Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
2026-03-16 23:07 ` Re: Avoid resource leak (contrib/postgres_fdw/connection.c) Fujii Masao <[email protected]>
@ 2026-03-18 17:42 ` Matheus Alcantara <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Matheus Alcantara @ 2026-03-18 17:42 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; Ranier Vilela <[email protected]>; +Cc: pgsql-hackers
On 16/03/26 20:07, Fujii Masao wrote:
> On Mon, Mar 16, 2026 at 8:46 PM Ranier Vilela <[email protected]> wrote:
>>
>> Hi.
>>
>> Per Coverity.
>>
>> CID 1645716: (#1 of 1): Resource leak (RESOURCE_LEAK)
>> 8. leaked_storage: Variable str going out of scope leaks the storage str.data points to.
>>
>> The function *postgres_fdw_connection* leaks the contents of
>> var str.data
>> Once that function *cstring_to_text* palloc the contents
>> must be necessary to free the var str.data.
>
> It seems that postgres_fdw_connection() is expected to be called by
> ForeignServerConnectionString() via OidFunctionCall3(),
> and to allocate memory (including str.data) in the FDWConnectionContext
> memory context created by ForeignServerConnectionString().
>
> After calling postgres_fdw_connection(), ForeignServerConnectionString()
> deletes FDWConnectionContext. So it seems to me that any memory
> allocated in that context, including str.data, would not leak. No?
>
Yeah, this sounds correct, but I'm wondering if the current pfree
calls on postgres_fdw_connection() is needed?
My previous review was assuming that any allocated memory on
postgres_fdw_connection() should be free at the end because of these
calls, but ForeignServerConnectionString() already assume that this
function leak memory:
/*
* GetForeignServer, GetForeignDataWrapper, and the connection function
* itself all leak memory into CurrentMemoryContext. Switch to a temporary
* context for easy cleanup.
*/
tempContext = AllocSetContextCreate(CurrentMemoryContext,
"FDWConnectionContext",
ALLOCSET_SMALL_SIZES);
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-03-18 17:42 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-16 11:45 Avoid resource leak (contrib/postgres_fdw/connection.c) Ranier Vilela <[email protected]>
2026-03-16 14:16 ` Matheus Alcantara <[email protected]>
2026-03-16 14:18 ` Ranier Vilela <[email protected]>
2026-03-16 23:07 ` Fujii Masao <[email protected]>
2026-03-18 17:42 ` Matheus Alcantara <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox