Thread: [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!
Attachment
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! > >
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! > >
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
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
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
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
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