[BUG] CRASH: ECPGprepared_statement() and ECPGdeallocate_all() when connection is NULL

Shruthi Gowda <gowdashru@gmail.com> writes:

The ECPG application crashes with a segmentation fault when calling
specific deallocation or prepared statement functions without an
established database connection. This is caused by a missing NULL check on
the connection handle before attempting to access it.

Hmm ... poking around, I see several other places that aren't checking
the result of ecpg_get_connection. Shouldn't we tighten them all?

regards, tom lane

On Mon, Dec 8, 2025 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Shruthi Gowda <gowdashru@gmail.com> writes:

The ECPG application crashes with a segmentation fault when calling
specific deallocation or prepared statement functions without an
established database connection. This is caused by a missing NULL check

on

the connection handle before attempting to access it.

Hmm ... poking around, I see several other places that aren't checking
the result of ecpg_get_connection. Shouldn't we tighten them all?

regards, tom lane

I agree. I’ve reviewed all occurrences of ecpg_get_connection() and noted
that, in most instances, it is followed by ecpg_init(), which validates the
connection and returns immediately if the connection is NULL.
In a few cases, the caller had already validated the connection. However, I
identified an additional case that lacked this check, so I have revised the
patch to include the missing validation.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 8, 2026 at 3:00 AM Shruthi Gowda <gowdashru@gmail.com> wrote:

On Mon, Dec 8, 2025 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Shruthi Gowda <gowdashru@gmail.com> writes:

The ECPG application crashes with a segmentation fault when calling
specific deallocation or prepared statement functions without an
established database connection. This is caused by a missing NULL check on
the connection handle before attempting to access it.

Hmm ... poking around, I see several other places that aren't checking
the result of ecpg_get_connection. Shouldn't we tighten them all?

regards, tom lane

I agree. I’ve reviewed all occurrences of ecpg_get_connection() and noted that, in most instances, it is followed by ecpg_init(), which validates the connection and returns immediately if the connection is NULL.

Why did you add this check instead of calling ecpg_init()?
Wouldn't it be better and sufficient to use ecpg_init() to validate
the connection?

+ con = ecpg_get_connection(connection_name);
+ if (!con)
+ {
+ ecpg_raise(lineno, ECPG_NO_CONN, ECPG_SQLSTATE_CONNECTION_DOES_NOT_EXIST,
+    connection_name ? connection_name : ecpg_gettext("NULL"));

Regards,

--
Fujii Masao

On Thu, Jan 8, 2026 at 9:32 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jan 8, 2026 at 3:00 AM Shruthi Gowda <gowdashru@gmail.com> wrote:

On Mon, Dec 8, 2025 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Shruthi Gowda <gowdashru@gmail.com> writes:

The ECPG application crashes with a segmentation fault when calling
specific deallocation or prepared statement functions without an
established database connection. This is caused by a missing NULL

check on

the connection handle before attempting to access it.

Hmm ... poking around, I see several other places that aren't checking
the result of ecpg_get_connection. Shouldn't we tighten them all?

regards, tom lane

I agree. I’ve reviewed all occurrences of ecpg_get_connection() and

noted that, in most instances, it is followed by ecpg_init(), which
validates the connection and returns immediately if the connection is NULL.

Why did you add this check instead of calling ecpg_init()?
Wouldn't it be better and sufficient to use ecpg_init() to validate
the connection?

+ con = ecpg_get_connection(connection_name);
+ if (!con)
+ {
+ ecpg_raise(lineno, ECPG_NO_CONN, ECPG_SQLSTATE_CONNECTION_DOES_NOT_EXIST,
+    connection_name ? connection_name : ecpg_gettext("NULL"));

Thanks for the feedback, Fujii. I agree—using ecpg_init() is a more
consistent approach and aligns with how this is handled in other parts of
the code.
I have updated the patch to use ecpg_init() for validation. Please find the
revised version attached.
The patch works for MASTER and all the back branches.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

On Mon, 19 Jan 2026 at 17:38, Shruthi Gowda <gowdashru@gmail.com> wrote:

On Thu, Jan 8, 2026 at 9:32 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jan 8, 2026 at 3:00 AM Shruthi Gowda <gowdashru@gmail.com> wrote:

On Mon, Dec 8, 2025 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Shruthi Gowda <gowdashru@gmail.com> writes:

The ECPG application crashes with a segmentation fault when calling
specific deallocation or prepared statement functions without an
established database connection. This is caused by a missing NULL check on
the connection handle before attempting to access it.

Hmm ... poking around, I see several other places that aren't checking
the result of ecpg_get_connection. Shouldn't we tighten them all?

regards, tom lane

I agree. I’ve reviewed all occurrences of ecpg_get_connection() and noted that, in most instances, it is followed by ecpg_init(), which validates the connection and returns immediately if the connection is NULL.

Why did you add this check instead of calling ecpg_init()?
Wouldn't it be better and sufficient to use ecpg_init() to validate
the connection?

+ con = ecpg_get_connection(connection_name);
+ if (!con)
+ {
+ ecpg_raise(lineno, ECPG_NO_CONN, ECPG_SQLSTATE_CONNECTION_DOES_NOT_EXIST,
+    connection_name ? connection_name : ecpg_gettext("NULL"));

Thanks for the feedback, Fujii. I agree—using ecpg_init() is a more consistent approach and aligns with how this is handled in other parts of the code.
I have updated the patch to use ecpg_init() for validation. Please find the revised version attached.
The patch works for MASTER and all the back branches.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

Thanks Shruthi for the updated patch.

Please add a crash test case in your patch. If possible, please add a
test for connection=NULL for ECPGdeallocate_all,
ECPGprepared_statement and ECPGget_desc.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

Here are some review comments on v3 patch:-

1.

Change in descriptor.c file - In my opinion, we can use `if(conn)` with
ecpg_raise, like other occurrence of ecpg_get_connection() call check in
this file, and not using ecpg_init(). Three reasons: a) Consistency in
checking conn after ecpg_get_connection() call in this file with if check.
b) We don't need to remove 'ecpg_init_sqlca(sqlca);' line due to call to
ecpg_init(). c) #2 comment below.
2.

If you agree with #1, then I see many other reasons for which
ECPGget_desc() returns and we can avoid ecpg_get_connection() call at top
of that function for those reasons and keep the check at the required
location only instead of moving at top of the function.
3.

I see there is one more location of ecpg_get_connection() call where
there is no check of NULL conn. In function ecpg_freeStmtCacheEntry() of
file prepare.c? I understand it's not required for a call in
ecpg_auto_prepare(), as the caller already validated that connection
string. But I think, conn in ecpg_freeStmtCacheEntry() is different from
the one that was validated.
4.

+1 to Mahindra, new test cases specific to the crash required for this
change?

Regards,
Nishant Sharma,
EDB, Pune.
https://www.enterprisedb.com/

On Tue, Mar 24, 2026 at 11:29 AM Nishant Sharma <
nishant.sharma@enterprisedb.com> wrote:

Here are some review comments on v3 patch:-

1.

Change in descriptor.c file - In my opinion, we can use `if(conn)`
with ecpg_raise, like other occurrence of ecpg_get_connection() call check
in this file, and not using ecpg_init(). Three reasons: a) Consistency in
checking conn after ecpg_get_connection() call in this file with if check.
b) We don't need to remove 'ecpg_init_sqlca(sqlca);' line due to call to
ecpg_init(). c) #2 comment below.
2.

If you agree with #1, then I see many other reasons for which
ECPGget_desc() returns and we can avoid ecpg_get_connection() call at top
of that function for those reasons and keep the check at the required
location only instead of moving at top of the function.
3.

I see there is one more location of ecpg_get_connection() call where
there is no check of NULL conn. In function ecpg_freeStmtCacheEntry() of
file prepare.c? I understand it's not required for a call in
ecpg_auto_prepare(), as the caller already validated that connection
string. But I think, conn in ecpg_freeStmtCacheEntry() is different from
the one that was validated.
4.

+1 to Mahindra, new test cases specific to the crash required for this
change?

Regards,
Nishant Sharma,
EDB, Pune.
https://www.enterprisedb.com/

Thanks, Nishant, for the review. I agree with points 1 and 2 and have
revised the patch accordingly. Regarding point 3, you are correct; the conn
in ecpg_freeStmtCacheEntry() differs from the one validated in the caller.
I have now added the necessary validation in the latest version.

I have also included a test case patch covering all execution paths except
for the ecpg_freeStmtCacheEntry() flow. I was unable to trigger that
specific flow, and it appears none of the existing test cases cover that
line either.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

Thanks Shruthi, for the new patches!

Here are some review comments:-
1. I think we should rename "test_null_connection.pgc" to "test6.pgc", and
make other required changes appropriately. We can see existing test files
uses test1.pgc, test2.pgc ..., test5.pgc as naming convention, and has test
description inside the file.
2. No need to add test_null_connection.c as commit file in
src/interfaces/ecpg/test/connect/ folder, we don't see such C src files for
other existing test files like test1.c, test2.c ..., test5.c. It is part of
src/interfaces/ecpg/test/connect/.gitignore. So, need to add test6.c and
its executable test6 like others in
src/interfaces/ecpg/test/connect/.gitignore
3. After doing #2 above make sure to create the test9.c C src file as
src/interfaces/ecpg/test/expected/connect-test6.c, I can see other existing
connect tests C src files in src/interfaces/ecpg/test/expected/ folder.
3. Not able to run "make -j 8 installcheck" on my setup, fails due to #3.
4. Extra space at the end (don't see that in other tested cases prints) :
printf("Test 1: Try to get descriptor on a disconnected connection \n");
5. char *query = "SELECT 1" -- unused in new test file added.
6. "if (stmt.connection == NULL)" --> "if (!stmt.connection)"?
7. Should we return -1 in else of new fix added in
ecpg_freeStmtCacheEntry()? If 'con' is NULL, why clean up the cache? Is
returning -1 more defensive than cleaning the cache?
8. Do we need to clean up "stmt.oldlocale = ecpg_strdup(...)" and undo
setlocale() before returning from the newly added fix in ECPGget_desc()?

Regards,
Nishant Sharma,
EDB, Pune.
https://www.enterprisedb.com/

On Thu, Apr 9, 2026 at 4:14 PM Shruthi Gowda <gowdashru@gmail.com> wrote:

Apologies, I top posted my previous review comments.

On Mon, Apr 13, 2026 at 4:00 PM Nishant Sharma <
nishant.sharma@enterprisedb.com> wrote:

Thanks Shruthi, for the new patches!

Here are some review comments:-
1. I think we should rename "test_null_connection.pgc" to "test6.pgc", and
make other required changes appropriately. We can see existing test files
uses test1.pgc, test2.pgc ..., test5.pgc as naming convention, and has test
description inside the file.
2. No need to add test_null_connection.c as commit file in
src/interfaces/ecpg/test/connect/ folder, we don't see such C src files for
other existing test files like test1.c, test2.c ..., test5.c. It is part of
src/interfaces/ecpg/test/connect/.gitignore. So, need to add test6.c and
its executable test6 like others in
src/interfaces/ecpg/test/connect/.gitignore
3. After doing #2 above make sure to create the test9.c C src file as
src/interfaces/ecpg/test/expected/connect-test6.c, I can see other existing
connect tests C src files in src/interfaces/ecpg/test/expected/ folder.
3. Not able to run "make -j 8 installcheck" on my setup, fails due to #3.
4. Extra space at the end (don't see that in other tested cases prints) :
printf("Test 1: Try to get descriptor on a disconnected connection \n");
5. char *query = "SELECT 1" -- unused in new test file added.
6. "if (stmt.connection == NULL)" --> "if (!stmt.connection)"?
7. Should we return -1 in else of new fix added in
ecpg_freeStmtCacheEntry()? If 'con' is NULL, why clean up the cache? Is
returning -1 more defensive than cleaning the cache?
8. Do we need to clean up "stmt.oldlocale = ecpg_strdup(...)" and undo
setlocale() before returning from the newly added fix in ECPGget_desc()?

Regards,
Nishant Sharma,
EDB, Pune.
https://www.enterprisedb.com/

On Thu, Apr 9, 2026 at 4:14 PM Shruthi Gowda <gowdashru@gmail.com> wrote:

On Tue, Mar 24, 2026 at 11:29 AM Nishant Sharma <
nishant.sharma@enterprisedb.com> wrote:

Here are some review comments on v3 patch:-

1.

Change in descriptor.c file - In my opinion, we can use `if(conn)`
with ecpg_raise, like other occurrence of ecpg_get_connection() call check
in this file, and not using ecpg_init(). Three reasons: a) Consistency in
checking conn after ecpg_get_connection() call in this file with if check.
b) We don't need to remove 'ecpg_init_sqlca(sqlca);' line due to call to
ecpg_init(). c) #2 comment below.
2.

If you agree with #1, then I see many other reasons for which
ECPGget_desc() returns and we can avoid ecpg_get_connection() call at top
of that function for those reasons and keep the check at the required
location only instead of moving at top of the function.
3.

I see there is one more location of ecpg_get_connection() call where
there is no check of NULL conn. In function ecpg_freeStmtCacheEntry() of
file prepare.c? I understand it's not required for a call in
ecpg_auto_prepare(), as the caller already validated that connection
string. But I think, conn in ecpg_freeStmtCacheEntry() is different from
the one that was validated.
4.

+1 to Mahindra, new test cases specific to the crash required for
this change?

Regards,
Nishant Sharma,
EDB, Pune.
https://www.enterprisedb.com/

Thanks, Nishant, for the review. I agree with points 1 and 2 and have
revised the patch accordingly. Regarding point 3, you are correct; the
conn in ecpg_freeStmtCacheEntry() differs from the one validated in the
caller. I have now added the necessary validation in the latest version.

I have also included a test case patch covering all execution paths
except for the ecpg_freeStmtCacheEntry() flow. I was unable to trigger
that specific flow, and it appears none of the existing test cases cover
that line either.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

Thanks for the review Nishant. I have updated the test case patch to
address comments 1–5. Regarding points 6–8, please see my detailed
responses below:
6. "if (stmt.connection == NULL)" --> "if (!stmt.connection)"?
--> Both formats are used interchangeably in the ecpg code. I’d prefer
to stick with the explicit null check here

7. Should we return -1 in else of new fix added in
ecpg_freeStmtCacheEntry()? If 'con' is NULL, why clean up the cache? Is
returning -1 more defensive than cleaning the cache?
--> The cache entry cleanup must happen regardless of whether the
connection exists, because:
- The cache slot is being reclaimed for a new statement
- Memory must be freed to avoid leaks

- A disconnected connection means the entry is stale and must be cleaned
The NULL check protects against crashes while still performing necessary
cleanup.

8. Do we need to clean up "stmt.oldlocale = ecpg_strdup(...)" and undo
setlocale() before returning from the newly added fix in ECPGget_desc()?
Great point! To avoid this cleanup complexity, I moved the connection
check before the locale setup so the early return happens before any
resources are allocated.

Thanks & Regards,

Shruthi K C

EnterpriseDB: http://www.enterprisedb.com

Thanks Shruthi for the new patches!

I can apply v5 on all active branches (i.e master, REL_18, REL_17, REL_16,
REL_15, REL_14).
I checked PG's ECPG regression with v5 and v2_test on master, REL_18,
REL_17, and REL_16 using both make and meson.

The v5 patch looks good to me now. I only have a few observations on
v2_test patch.

Review comments on v2_test:
1. Not able to apply the v2 test patch on PG15 and PG14 branch, appears
meson.build for test is not there on these branches. Need to remove all
meson related changes for these branches.
2. char val2[5] = "data1"; --> val2's size will not be able to fit '\0' for
the string. As size of val2 and number of characters in "data1" are exactly
same?
3. "exec sql disconnect;" - Do we need this at the end as we have already
disconnected before Test 2.
3. Minor - Do we need "printf("sqlca.sqlcode = %ld\n", sqlca.sqlcode);"? I
see all others have it, but not Test 4?

Regards,
Nishant Sharma,
Pune, EDB.
https://www.enterprisedb.com/