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

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(at)gmail(dot)com> wrote:

>
>
> On Tue, Mar 24, 2026 at 11:29 AM Nishant Sharma <
> nishant(dot)sharma(at)enterprisedb(dot)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
>
>