public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nishant Sharma <[email protected]>
To: Shruthi Gowda <[email protected]>
Cc: Mahendra Singh Thalor <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL Development <[email protected]>
Subject: Re: [BUG] CRASH: ECPGprepared_statement() and ECPGdeallocate_all() when connection is NULL
Date: Mon, 13 Apr 2026 16:00:00 +0530
Message-ID: <CADrsxdbPw1ZYcuqH1-DTNhAvRN=tRTTY+_dFy8wU2g4DQb67Bg@mail.gmail.com> (raw)
In-Reply-To: <CAASxf_OdsJRi17EZ_ZMyQgOwUzn357YyMqJ2Z2qiExPaLRW_Lg@mail.gmail.com>
References: <CAASxf_P1F75Ck+0qyb10auT+BORupOM4yigXBnm7aWRNx1LYcA@mail.gmail.com>
<[email protected]>
<CAASxf_P5f=Frf8S7rN9BzphtCLoeN9vFuh-V7ukotOQZU54g+w@mail.gmail.com>
<CAHGQGwHAPXexiGaHMkDDRF7cPBr_3fgCNdT4n2+1UjaEU++BAQ@mail.gmail.com>
<CAASxf_OGWD7PA5TMEh2MdF2YxN8V3ByLhnFJ=uw0hKr33sgqAw@mail.gmail.com>
<CAKYtNAqjJbzV+ZJDqA-s0fHSLen6msc=A0SfrTS1ub1KKH9haQ@mail.gmail.com>
<CADrsxdbb2fn1LACQShrQT0bNqSCQ3hSzEojb2tODhD0PmewDiA@mail.gmail.com>
<CAASxf_OdsJRi17EZ_ZMyQgOwUzn357YyMqJ2Z2qiExPaLRW_Lg@mail.gmail.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 <[email protected]> wrote:
>
>
> On Tue, Mar 24, 2026 at 11:29 AM Nishant Sharma <
> [email protected]> 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
>
>
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [BUG] CRASH: ECPGprepared_statement() and ECPGdeallocate_all() when connection is NULL
In-Reply-To: <CADrsxdbPw1ZYcuqH1-DTNhAvRN=tRTTY+_dFy8wU2g4DQb67Bg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox