Thread: [PATCH] memory leak in ecpglib

[PATCH] memory leak in ecpglib

From
"Zhang, Jie"
Date:
Hi all

Memory leaks occur when the ecpg_update_declare_statement() is called the second time.

FILE:postgresql\src\interfaces\ecpg\ecpglib\prepare.c
void
ecpg_update_declare_statement(const char *declared_name, const char *cursor_name, const int lineno)
{
    struct declared_statement *p = NULL;

    if (!declared_name || !cursor_name)
        return;

    /* Find the declared node by declared name */
    p = ecpg_find_declared_statement(declared_name);
    if (p)
        p->cursor_name = ecpg_strdup(cursor_name, lineno);  ★
}
ecpg_strdup() returns a pointer to a null-terminated byte string, which is a duplicate of the string pointed to by
str.
The memory obtained is done dynamically using malloc and hence it can be freed using free().

When the ecpg_update_declare_statement() is called for the second time, 
the memory allocated for p->cursor_name is not freed. 

For example:

    EXEC SQL BEGIN DECLARE SECTION;
        char *selectString = "SELECT * FROM foo;";
        int FooBar;
        char DooDad[17];
    EXEC SQL END DECLARE SECTION;

    EXEC SQL CONNECT TO postgres@localhost:5432 AS con1 USER postgres;

    EXEC SQL AT con1 DECLARE stmt_1 STATEMENT;
    EXEC SQL AT con1 PREPARE stmt_1 FROM :selectString;

    EXEC SQL AT con1 DECLARE cur_1 CURSOR FOR stmt_1; //★1     ECPGopen() --> ecpg_update_declare_statement()
    EXEC SQL AT con1 OPEN cur_1;   

    EXEC SQL AT con1 DECLARE cur_2 CURSOR FOR stmt_1; //★2     ECPGopen() --> ecpg_update_declare_statement()
    EXEC SQL AT con1 OPEN cur_2;                                                  Memory leaks

    EXEC SQL FETCH cur_2 INTO:FooBar, :DooDad;
    EXEC SQL COMMIT;
    EXEC SQL DISCONNECT ALL;


We should free p->cursor_name before p->cursor_name = ecpg_strdup(cursor_name, lineno).
#############################################################################
        if(p->cursor_name)
            ecpg_free(p->cursor_name);
        p->cursor_name = ecpg_strdup(cursor_name,lineno);
###########################################################################
Here is a patch.

Best Regards!




Attachment

RE: [PATCH] memory leak in ecpglib

From
"Matsumura, Ryo"
Date:
Hi

On Mon. June. 10, 2019 at 09:54 AM Zhang, Jie
< zhangjie2@cn.fujitsu.com > wrote:
> 
> Memory leaks occur when the ecpg_update_declare_statement() is called the
> second time.

Certainly it is.
But I wonder if it is safe that the old cursor_name is forgotten.

Regards
Ryo Matsumura


> -----Original Message-----
> From: Zhang, Jie [mailto:zhangjie2@cn.fujitsu.com]
> Sent: Monday, June 10, 2019 9:54 AM
> To: pgsql-hackers@lists.postgresql.org
> Cc: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>
> Subject: [PATCH] memory leak in ecpglib
> 
> Hi all
> 
> Memory leaks occur when the ecpg_update_declare_statement() is called the
> second time.
> 
> FILE:postgresql\src\interfaces\ecpg\ecpglib\prepare.c
> void
> ecpg_update_declare_statement(const char *declared_name, const char
> *cursor_name, const int lineno)
> {
>     struct declared_statement *p = NULL;
> 
>     if (!declared_name || !cursor_name)
>         return;
> 
>     /* Find the declared node by declared name */
>     p = ecpg_find_declared_statement(declared_name);
>     if (p)
>         p->cursor_name = ecpg_strdup(cursor_name, lineno);  ★
> }
> ecpg_strdup() returns a pointer to a null-terminated byte string, which is
> a duplicate of the string pointed to by str.
> The memory obtained is done dynamically using malloc and hence it can be freed
> using free().
> 
> When the ecpg_update_declare_statement() is called for the second time,
> the memory allocated for p->cursor_name is not freed.
> 
> For example:
> 
>     EXEC SQL BEGIN DECLARE SECTION;
>         char *selectString = "SELECT * FROM foo;";
>         int FooBar;
>         char DooDad[17];
>     EXEC SQL END DECLARE SECTION;
> 
>     EXEC SQL CONNECT TO postgres@localhost:5432 AS con1 USER postgres;
> 
>     EXEC SQL AT con1 DECLARE stmt_1 STATEMENT;
>     EXEC SQL AT con1 PREPARE stmt_1 FROM :selectString;
> 
>     EXEC SQL AT con1 DECLARE cur_1 CURSOR FOR stmt_1; //★1     ECPGopen()
> --> ecpg_update_declare_statement()
>     EXEC SQL AT con1 OPEN cur_1;
> 
>     EXEC SQL AT con1 DECLARE cur_2 CURSOR FOR stmt_1; //★2     ECPGopen()
> --> ecpg_update_declare_statement()
>     EXEC SQL AT con1 OPEN cur_2;
> Memory leaks
> 
>     EXEC SQL FETCH cur_2 INTO:FooBar, :DooDad;
>     EXEC SQL COMMIT;
>     EXEC SQL DISCONNECT ALL;
> 
> 
> We should free p->cursor_name before p->cursor_name = ecpg_strdup(cursor_name,
> lineno).
> #########################################################################
> ####
>         if(p->cursor_name)
>             ecpg_free(p->cursor_name);
>         p->cursor_name = ecpg_strdup(cursor_name,lineno);
> #########################################################################
> ##
> Here is a patch.
> 
> Best Regards!
> 
> 


RE: [PATCH] memory leak in ecpglib

From
"Zhang, Jie"
Date:
Hi

> But I wonder if it is safe that the old cursor_name is forgotten.
old cursor_name is not assigned to other pointers, so it is safe that the old cursor_name is forgotten.

Best Regards!

-----Original Message-----
From: Matsumura, Ryo/松村 量 
Sent: Monday, June 10, 2019 5:52 PM
To: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>; pgsql-hackers@lists.postgresql.org
Cc: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>
Subject: RE: [PATCH] memory leak in ecpglib

Hi

On Mon. June. 10, 2019 at 09:54 AM Zhang, Jie < zhangjie2@cn.fujitsu.com > wrote:
> 
> Memory leaks occur when the ecpg_update_declare_statement() is called 
> the second time.

Certainly it is.
But I wonder if it is safe that the old cursor_name is forgotten.

Regards
Ryo Matsumura


> -----Original Message-----
> From: Zhang, Jie [mailto:zhangjie2@cn.fujitsu.com]
> Sent: Monday, June 10, 2019 9:54 AM
> To: pgsql-hackers@lists.postgresql.org
> Cc: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>
> Subject: [PATCH] memory leak in ecpglib
> 
> Hi all
> 
> Memory leaks occur when the ecpg_update_declare_statement() is called 
> the second time.
> 
> FILE:postgresql\src\interfaces\ecpg\ecpglib\prepare.c
> void
> ecpg_update_declare_statement(const char *declared_name, const char 
> *cursor_name, const int lineno) {
>     struct declared_statement *p = NULL;
> 
>     if (!declared_name || !cursor_name)
>         return;
> 
>     /* Find the declared node by declared name */
>     p = ecpg_find_declared_statement(declared_name);
>     if (p)
>         p->cursor_name = ecpg_strdup(cursor_name, lineno);  ★ }
> ecpg_strdup() returns a pointer to a null-terminated byte string, 
> which is a duplicate of the string pointed to by str.
> The memory obtained is done dynamically using malloc and hence it can 
> be freed using free().
> 
> When the ecpg_update_declare_statement() is called for the second 
> time, the memory allocated for p->cursor_name is not freed.
> 
> For example:
> 
>     EXEC SQL BEGIN DECLARE SECTION;
>         char *selectString = "SELECT * FROM foo;";
>         int FooBar;
>         char DooDad[17];
>     EXEC SQL END DECLARE SECTION;
> 
>     EXEC SQL CONNECT TO postgres@localhost:5432 AS con1 USER postgres;
> 
>     EXEC SQL AT con1 DECLARE stmt_1 STATEMENT;
>     EXEC SQL AT con1 PREPARE stmt_1 FROM :selectString;
> 
>     EXEC SQL AT con1 DECLARE cur_1 CURSOR FOR stmt_1; //★1     ECPGopen()
> --> ecpg_update_declare_statement()
>     EXEC SQL AT con1 OPEN cur_1;
> 
>     EXEC SQL AT con1 DECLARE cur_2 CURSOR FOR stmt_1; //★2     ECPGopen()
> --> ecpg_update_declare_statement()
>     EXEC SQL AT con1 OPEN cur_2;
> Memory leaks
> 
>     EXEC SQL FETCH cur_2 INTO:FooBar, :DooDad;
>     EXEC SQL COMMIT;
>     EXEC SQL DISCONNECT ALL;
> 
> 
> We should free p->cursor_name before p->cursor_name = 
> ecpg_strdup(cursor_name, lineno).
> ######################################################################
> ###
> ####
>         if(p->cursor_name)
>             ecpg_free(p->cursor_name);
>         p->cursor_name = ecpg_strdup(cursor_name,lineno); 
> ######################################################################
> ###
> ##
> Here is a patch.
> 
> Best Regards!
> 
> 




RE: [PATCH] memory leak in ecpglib

From
"kuroda.hayato@fujitsu.com"
Date:
Dear Zhang,

# I resend the email

Thank you for reporting a bug. I didn't care about this case.

>> We should free p->cursor_name before p->cursor_name = 
>> ecpg_strdup(cursor_name, lineno).

I'm wondering whether this approach is correct or not.
If your patch is committed, in your example, any operation for cur1 will not be accepted.

My idea is changing ecpg_update_declare_statement() for permitting one-to-many relation between a declared name and
cursors.
An example is as below:

p = ecpg_find_declared_statement(declared_name);
if (p && p->cursor_name == cursor_name)
    p->cursor_name = ecpg_strdup(cursor_name, lineno);

Do you have any suggestions or comments for this?

Best Regards,
Hayato Kuroda
Fujitsu LIMITED

RE: [PATCH] memory leak in ecpglib

From
"Zhang, Jie"
Date:
Hi Kuroda,

>If your patch is committed, in your example, any operation for cur1 will not be accepted.
Although the return value after calling ecpg_get_con_name_by_cursor_name(cur1) is NULL,
in ecpg_get_connection(), actual_connection will be returned.
so, operation for cur1 will be accepted,

>p = ecpg_find_declared_statement(declared_name);
>if (p && p->cursor_name == cursor_name)
>p->cursor_name = ecpg_strdup(cursor_name, lineno);
Because the initial value of p->cursor_name is NULL, p->cursor_name will never be updated.

Best Regards!

-----Original Message-----
From: Kuroda, Hayato/�\田 隼人 
Sent: Tuesday, June 11, 2019 2:36 PM
To: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>; Matsumura, Ryo/松村 量 <matsumura.ryo@jp.fujitsu.com>;
pgsql-hackers@lists.postgresql.org
Subject: RE: [PATCH] memory leak in ecpglib

Dear Zhang,

# I resend the email

Thank you for reporting a bug. I didn't care about this case.

>> We should free p->cursor_name before p->cursor_name = 
>> ecpg_strdup(cursor_name, lineno).

I'm wondering whether this approach is correct or not.
If your patch is committed, in your example, any operation for cur1 will not be accepted.

My idea is changing ecpg_update_declare_statement() for permitting one-to-many relation between a declared name and
cursors.
An example is as below:

p = ecpg_find_declared_statement(declared_name);
if (p && p->cursor_name == cursor_name)
p->cursor_name = ecpg_strdup(cursor_name, lineno);

Do you have any suggestions or comments for this?

Best Regards,
Hayato Kuroda
Fujitsu LIMITED




RE: [PATCH] memory leak in ecpglib

From
"kuroda.hayato@fujitsu.com"
Date:
Dear Zhang,

Sorry for my late reply.
I'm now planning to refactor this functionality:
https://www.postgresql.org/message-id/OSAPR01MB20048298F882D25897C6AB23F5EF0@OSAPR01MB2004.jpnprd01.prod.outlook.com

If DECLARE STATEMENT and other related statements are enabled only preprocessing process, this problem will be easily
solved.

How about it?

Hayato Kuroda
Fujitsu LIMITED


-----Original Message-----
From: Zhang, Jie/张 杰 
Sent: Tuesday, June 11, 2019 5:14 PM
To: Kuroda, Hayato/�\田 隼人 <kuroda.hayato@fujitsu.com>; Matsumura, Ryo/松村 量 <matsumura.ryo@jp.fujitsu.com>;
pgsql-hackers@lists.postgresql.org
Subject: RE: [PATCH] memory leak in ecpglib

Hi Kuroda,

>If your patch is committed, in your example, any operation for cur1 will not be accepted.
Although the return value after calling ecpg_get_con_name_by_cursor_name(cur1) is NULL,
in ecpg_get_connection(), actual_connection will be returned.
so, operation for cur1 will be accepted,

>p = ecpg_find_declared_statement(declared_name);
>if (p && p->cursor_name == cursor_name)
>p->cursor_name = ecpg_strdup(cursor_name, lineno);
Because the initial value of p->cursor_name is NULL, p->cursor_name will never be updated.

Best Regards!

-----Original Message-----
From: Kuroda, Hayato/�\田 隼人 
Sent: Tuesday, June 11, 2019 2:36 PM
To: Zhang, Jie/张 杰 <zhangjie2@cn.fujitsu.com>; Matsumura, Ryo/松村 量 <matsumura.ryo@jp.fujitsu.com>;
pgsql-hackers@lists.postgresql.org
Subject: RE: [PATCH] memory leak in ecpglib

Dear Zhang,

# I resend the email

Thank you for reporting a bug. I didn't care about this case.

>> We should free p->cursor_name before p->cursor_name = 
>> ecpg_strdup(cursor_name, lineno).

I'm wondering whether this approach is correct or not.
If your patch is committed, in your example, any operation for cur1 will not be accepted.

My idea is changing ecpg_update_declare_statement() for permitting one-to-many relation between a declared name and
cursors.
An example is as below:

p = ecpg_find_declared_statement(declared_name);
if (p && p->cursor_name == cursor_name)
p->cursor_name = ecpg_strdup(cursor_name, lineno);

Do you have any suggestions or comments for this?

Best Regards,
Hayato Kuroda
Fujitsu LIMITED



Re: [PATCH] memory leak in ecpglib

From
Michael Meskes
Date:
Hi,

> Memory leaks occur when the ecpg_update_declare_statement() is called
> the second time.
> ...

I'm going to commit this patch HEAD, this way we can see if it works as
advertised. It does not hurt if it gets removed by a rewrite.

Thanks for finding the issue,

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL




RE: [PATCH] memory leak in ecpglib

From
"kuroda.hayato@fujitsu.com"
Date:
Dear Meskes, Zhang,

I think this modification is not enough, and I have an another idea. 

>> If your patch is committed, in your example, any operation for cur1 will not be accepted.
> Although the return value after calling ecpg_get_con_name_by_cursor_name(cur1)
> is NULL, in ecpg_get_connection(), actual_connection will be returned.
> so, operation for cur1 will be accepted,

Did you mention about this code?
(Copied from ECPGfetch)

```
real_connection_name = ecpg_get_con_name_by_cursor_name(cursor_name);
if (real_connection_name == NULL)
{
    /*
    * If can't get the connection name by cursor name then using
    * connection name coming from the parameter connection_name
    */
    real_connection_name = connection_name;
}
```

If so, I think this approach is wrong. This connection_name corresponds to the following con1.

```
EXEC SQL AT con1 FETCH cur1 ...
            ^^^^
```

Therefore the followed FETCH statement will fail
because the application forget the connection of cur_1.

```
EXEC SQL AT con1 DECLARE stmt_1 STATEMENT;
EXEC SQL PREPARE stmt_1 FROM :selectString;
EXEC SQL DECLARE cur_1 CURSOR FOR stmt_1;
EXEC SQL OPEN cur_1;   
EXEC SQL DECLARE cur_2 CURSOR FOR stmt_1;
EXEC SQL OPEN cur_2;
EXEC SQL FETCH cur_1;
```


I think the g_declared_list is not needed for managing connection. I was wrong.
We should treat DECLARE STAEMENT as declarative, like #include or #define in C macro.

Please send me your reply.

Best Regards,
Hayato Kuroda
Fujitsu LIMITED