public inbox for [email protected]help / color / mirror / Atom feed
Avoid 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]> 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 14:16 Matheus Alcantara <[email protected]> parent: 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 14:18 Ranier Vilela <[email protected]> parent: Matheus Alcantara <[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 23:07 Fujii Masao <[email protected]> parent: Ranier Vilela <[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-18 17:42 Matheus Alcantara <[email protected]> parent: Fujii Masao <[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