[PATCH] memory leak in ecpglib

Started by Zhang, Jieover 6 years ago8 messages

zhangjie2@cn.fujitsu.com

over 6 years ago

1 attachment(s)

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!

Attachments:

ecpglib.patchapplication/octet-stream; name=ecpglib.patchDownload

diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index 0dcf736..6edff5c 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -754,7 +754,11 @@ ecpg_update_declare_statement(const char *declared_name, const char *cursor_name
 	/* Find the declared node by declared name */
 	p = ecpg_find_declared_statement(declared_name);
 	if (p)
+	{
+		if (p->cursor_name)
+			ecpg_free(p->cursor_name);
 		p->cursor_name = ecpg_strdup(cursor_name, lineno);
+	}
 }
 
 /*

Matsumura, Ryo

matsumura.ryo@jp.fujitsu.com

over 6 years ago

In reply to: Zhang, Jie (#1)

RE: [PATCH] memory leak in ecpglib

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

Show quoted text

-----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!

Zhang, Jie

zhangjie2@cn.fujitsu.com

over 6 years ago

In reply to: Matsumura, Ryo (#2)

RE: [PATCH] memory leak in ecpglib

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

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

Show quoted text

-----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!

kuroda.hayato@fujitsu.com

over 6 years ago

In reply to: Zhang, Jie (#3)

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

Zhang, Jie

zhangjie2@cn.fujitsu.com

over 6 years ago

In reply to: kuroda.hayato@fujitsu.com (#4)

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

kuroda.hayato@fujitsu.com

over 6 years ago

In reply to: Zhang, Jie (#5)

RE: [PATCH] memory leak in ecpglib

Dear Zhang,

Sorry for my late reply.
I'm now planning to refactor this functionality:
/messages/by-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!

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

Michael Meskes

meskes@postgresql.org

over 6 years ago

In reply to: Zhang, Jie (#1)

Re: [PATCH] memory leak in ecpglib

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

kuroda.hayato@fujitsu.com

over 6 years ago

In reply to: Michael Meskes (#7)

RE: [PATCH] memory leak in ecpglib

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