Small memory leak in execute.c of ECPG driver
Hi all,
In exactly 3 places of the ECPG driver (for numeric, for interval and
for date), we do something as follows:
/* Allocation of mallocedval */
if (!(mallocedval = ecpg_strdup("array [", lineno)))
return false;
for (element = 0; element < var->arrsize; element++)
{
int result;
ptr = stuff_alloc();
if (!ptr)
return false; <= Leak here of mallocedval
It happens that if the allocation done within this for loop fails we
leak mallocedval that was previously allocated. Attached is a patch to
fix this issue spotted by Coverity.
Regards
--
Michael
Attachments:
0001-Fix-memory-leak-in-ecpg-driver.patchapplication/x-patch; name=0001-Fix-memory-leak-in-ecpg-driver.patchDownload
From 5911fadddbf78d6d98f1d679e7ff2e78f9728185 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Tue, 3 Feb 2015 15:48:16 +0900
Subject: [PATCH] Fix memory leak in ecpg driver
Issue pointed out by Coverity.
---
src/interfaces/ecpg/ecpglib/execute.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 8a3dd75..abe60a5 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -859,7 +859,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
nval = PGTYPESnumeric_new();
if (!nval)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
if (var->type == ECPGt_numeric)
result = PGTYPESnumeric_copy((numeric *) ((var + var->offset * element)->value), nval);
@@ -940,7 +943,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
str = quote_postgres(PGTYPESinterval_to_asc((interval *) ((var + var->offset * element)->value)), quote, lineno);
if (!str)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
slen = strlen(str);
if (!(mallocedval = ecpg_realloc(mallocedval, strlen(mallocedval) + slen + 2, lineno)))
@@ -991,7 +997,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
str = quote_postgres(PGTYPESdate_to_asc(*(date *) ((var + var->offset * element)->value)), quote, lineno);
if (!str)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
slen = strlen(str);
if (!(mallocedval = ecpg_realloc(mallocedval, strlen(mallocedval) + slen + 2, lineno)))
--
2.2.2
On 02/03/2015 08:58 AM, Michael Paquier wrote:
Hi all,
In exactly 3 places of the ECPG driver (for numeric, for interval and
for date), we do something as follows:
/* Allocation of mallocedval */
if (!(mallocedval = ecpg_strdup("array [", lineno)))
return false;for (element = 0; element < var->arrsize; element++)
{
int result;ptr = stuff_alloc();
if (!ptr)
return false; <= Leak here of mallocedvalIt happens that if the allocation done within this for loop fails we
leak mallocedval that was previously allocated. Attached is a patch to
fix this issue spotted by Coverity.
I think there are more similar leaks nearby. After the first hunk,
there's another if-check with "return false" that also leaks
mallocedval. Right after the two other hunks, if the ecpg_realloc fails,
we again leak mallocedval.
I wonder why Coverity didn't warn about those? Maybe it would've, after
fixing the first ones.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Feb 3, 2015 at 5:35 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
I think there are more similar leaks nearby. After the first hunk, there's
another if-check with "return false" that also leaks mallocedval. Right
after the two other hunks, if the ecpg_realloc fails, we again leak
mallocedval.
Yes, I found some extra ones by re-reading the code again with newcopy
(2) as well as mallocedval (1) as you mentioned.
I wonder why Coverity didn't warn about those? Maybe it would've, after
fixing the first ones.
Hard to say. Perhaps it gives up after finding one failure in a code
path, or perhaps it would have found it after a version update.. In
any case, an updated patch is attached.
--
Michael
Attachments:
20150203_ecpg_leakfix_v2.patchapplication/x-patch; name=20150203_ecpg_leakfix_v2.patchDownload
From 4e195f162d879bf563fec052710dd10ccdc4a89a Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Tue, 3 Feb 2015 15:48:16 +0900
Subject: [PATCH] Fix memory leak in ecpg driver
Issue pointed out by Coverity.
---
src/interfaces/ecpg/ecpglib/execute.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 8a3dd75..5d26af7 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -803,7 +803,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
mallocedval = quote_postgres(newcopy, quote, lineno);
if (!mallocedval)
+ {
+ ecpg_free(newcopy);
return false;
+ }
*tobeinserted_p = mallocedval;
}
@@ -835,7 +838,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
mallocedval = quote_postgres(newcopy, quote, lineno);
if (!mallocedval)
+ {
+ ecpg_free(newcopy);
return false;
+ }
*tobeinserted_p = mallocedval;
}
@@ -859,7 +865,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
nval = PGTYPESnumeric_new();
if (!nval)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
if (var->type == ECPGt_numeric)
result = PGTYPESnumeric_copy((numeric *) ((var + var->offset * element)->value), nval);
@@ -869,6 +878,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
if (result != 0)
{
PGTYPESnumeric_free(nval);
+ ecpg_free(mallocedval);
return false;
}
@@ -940,7 +950,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
str = quote_postgres(PGTYPESinterval_to_asc((interval *) ((var + var->offset * element)->value)), quote, lineno);
if (!str)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
slen = strlen(str);
if (!(mallocedval = ecpg_realloc(mallocedval, strlen(mallocedval) + slen + 2, lineno)))
@@ -991,7 +1004,10 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
str = quote_postgres(PGTYPESdate_to_asc(*(date *) ((var + var->offset * element)->value)), quote, lineno);
if (!str)
+ {
+ ecpg_free(mallocedval);
return false;
+ }
slen = strlen(str);
if (!(mallocedval = ecpg_realloc(mallocedval, strlen(mallocedval) + slen + 2, lineno)))
--
2.2.2
On 02/03/2015 02:45 PM, Michael Paquier wrote:
On Tue, Feb 3, 2015 at 5:35 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:I think there are more similar leaks nearby. After the first hunk, there's
another if-check with "return false" that also leaks mallocedval. Right
after the two other hunks, if the ecpg_realloc fails, we again leak
mallocedval.Yes, I found some extra ones by re-reading the code again with newcopy
(2) as well as mallocedval (1) as you mentioned.
Committed, along with more fixes for leaks on ecpg_realloc failures.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers