| From: | Shruthi Gowda <gowdashru(at)gmail(dot)com> |
|---|---|
| To: | Nishant Sharma <nishant(dot)sharma(at)enterprisedb(dot)com> |
| Cc: | Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [BUG] CRASH: ECPGprepared_statement() and ECPGdeallocate_all() when connection is NULL |
| Date: | 2026-04-09 10:44:13 |
| Message-ID: | CAASxf_OdsJRi17EZ_ZMyQgOwUzn357YyMqJ2Z2qiExPaLRW_Lg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0001-Add-missing-connection-validation-in-ECPG.patch | application/octet-stream | 3.1 KB |
| v1_test-0001-Tests-for-NULL-connection-validation.patch | application/octet-stream | 13.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Dagfinn Ilmari Mannsåker | 2026-04-09 10:51:05 | pgcrypto: remove useless px_memset() and BF_ASM |
| Previous Message | Peter Eisentraut | 2026-04-09 10:42:52 | Re: [[deprecated("don't call this, call that")]] |