On Mon, Apr 13, 2026 at 4:00 PM Nishant Sharma < [email protected]> 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 <[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
>>
>
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
v5-0001-Add-missing-connection-validation-in-ECPG.patch
Description: Binary data
v2_test-0001-Tests-for-NULL-connection-validation.patch
Description: Binary data
