Re: [BUG] CRASH: ECPGprepared_statement() and ECPGdeallocate_all() when connection is NULL - Mailing list pgsql-hackers

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