Re: [HACKERS] Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done
On Tue, Feb 03, 2015 at 10:44:17PM +0900, Michael Paquier wrote: All those things are addressed in the patch attached. Fixed a typo and commited. Thanks Michael for fixing and Heikki for reviewing. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done
On Tue, Feb 3, 2015 at 7:50 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
Hmm. Since the ecpg_add_mem call is done after setting (*(void **) var),
that's left to point to already-free'd memory. The other call sites have a
similar issue. I haven't analyzed the code to check if that's harmless or
not, but seems better to not do that.
Right, it is an error do allocate this memory if there is a risk that
it may be freed. Hence fixed.
In ecpg_add_mem, the ecpg_raise() call is unnecessary, since ecpg_alloc
already does that on failure.
Right, check.
(It would be less error-prone to have an ecpg_alloc_auto() function that
combines ecpg_alloc+ecpg_add_mem in one call.)
It makes sense.
/* Here are some methods used by the lib. */
/* Returns a pointer to a string containing a simple type name. */
That second comment is completely bogus. It's not this patch's fault, it's
been like that forever, but just happened to notice..
Corrected.
All those things are addressed in the patch attached.
--
Michael
From 206dc0dd95a83e056968ac822f7b9331ffcf82cd Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Tue, 3 Feb 2015 16:21:50 +0900
Subject: [PATCH] Fix unlikely-to-happen crash in ecpg_add_mem
This routine was called ecpg_alloc to allocate to memory but did not
actually check the returned pointer allocated, potentially NULL which
is actually the result of a malloc call.
Issue noted by Coverity, though I guessed the legwork needed here.
---
src/interfaces/ecpg/ecpglib/descriptor.c | 6 ++
src/interfaces/ecpg/ecpglib/execute.c| 8 +++-
src/interfaces/ecpg/ecpglib/extern.h | 4 ++--
src/interfaces/ecpg/ecpglib/memory.c | 22 +-
4 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index b2990ca..956c035 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -432,7 +432,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
/* allocate storage if needed */
if (arrsize == 0 *(void **) var == NULL)
{
- void *mem = (void *) ecpg_alloc(offset * ntuples, lineno);
+ void *mem = (void *) ecpg_auto_alloc(offset * ntuples, lineno);
if (!mem)
{
@@ -440,7 +440,6 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
return false;
}
*(void **) var = mem;
- ecpg_add_mem(mem, lineno);
var = mem;
}
@@ -510,7 +509,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
/* allocate storage if needed */
if (data_var.ind_arrsize == 0 data_var.ind_value == NULL)
{
- void *mem = (void *) ecpg_alloc(data_var.ind_offset * ntuples, lineno);
+ void *mem = (void *) ecpg_auto_alloc(data_var.ind_offset * ntuples, lineno);
if (!mem)
{
@@ -518,7 +517,6 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
return false;
}
*(void **) data_var.ind_pointer = mem;
- ecpg_add_mem(mem, lineno);
data_var.ind_value = mem;
}
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 8a3dd75..46d884f 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -398,11 +398,10 @@ ecpg_store_result(const PGresult *results, int act_field,
}
ecpg_log(ecpg_store_result on line %d: allocating memory for %d tuples\n, stmt-lineno, ntuples);
- var-value = (char *) ecpg_alloc(len, stmt-lineno);
+ var-value = (char *) ecpg_auto_alloc(len, stmt-lineno);
if (!var-value)
return false;
*((char **) var-pointer) = var-value;
- ecpg_add_mem(var-value, stmt-lineno);
}
/* allocate indicator variable if needed */
@@ -410,11 +409,10 @@ ecpg_store_result(const PGresult *results, int act_field,
{
int len = var-ind_offset * ntuples;
- var-ind_value = (char *) ecpg_alloc(len, stmt-lineno);
- if (!var-ind_value)
+ var-ind_value = (char *) ecpg_auto_alloc(len, stmt-lineno);
+ if (!var_ind_value)
return false;
*((char **) var-ind_pointer) = var-ind_value;
- ecpg_add_mem(var-ind_value, stmt-lineno);
}
/* fill the variable with the tuple(s) */
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index 3836007..2b670e0 100644
--- a/src/interfaces/ecpg/ecpglib/extern.h
+++ b/src/interfaces/ecpg/ecpglib/extern.h
@@ -136,8 +136,7 @@ extern struct var_list *ivlist;
/* Here are some methods used by the lib. */
-/* Returns a pointer to a string containing a simple type name. */
-void ecpg_add_mem(void *ptr, int lineno);
+bool ecpg_add_mem(void *ptr, int lineno);
bool ecpg_get_data(const PGresult *, int, int, int, enum ECPGttype type,
enum ECPGttype, char *, char *, long, long, long,
@@ -148,6 +147,7 @@ void ecpg_pthreads_init(void);
#endif
struct connection *ecpg_get_connection(const char *);
char
Re: [HACKERS] Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done
On 02/03/2015 09:28 AM, Michael Paquier wrote:
Hi all,
In ecpg_add_mem of memory.c, we use ecpg_alloc but there is actually
no NULL-pointer check. If an OOM shows up exactly at this point, this
is likely to cause a crash. Attached patch adds some extra processing
to ecpg_add_mem to check if the allocation fails, and to fail properly
if an OOM appears.
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -440,7 +440,12 @@ ECPGget_desc(int lineno, const char *desc_name, int
index,...)
return false;
}
*(void **) var = mem;
- ecpg_add_mem(mem, lineno);
+ if (!ecpg_add_mem(mem, lineno))
+ {
+ ecpg_free(mem);
+ va_end(args);
+ return false;
+ }
var = mem;
}
Hmm. Since the ecpg_add_mem call is done after setting (*(void **) var),
that's left to point to already-free'd memory. The other call sites have
a similar issue. I haven't analyzed the code to check if that's harmless
or not, but seems better to not do that.
In ecpg_add_mem, the ecpg_raise() call is unnecessary, since ecpg_alloc
already does that on failure.
(It would be less error-prone to have an ecpg_alloc_auto() function that
combines ecpg_alloc+ecpg_add_mem in one call.)
/* Here are some methods used by the lib. */
/* Returns a pointer to a string containing a simple type name. */
boolecpg_add_mem(void *ptr, int lineno);
bool ecpg_get_data(const PGresult *, int, int, int, enum ECPGttype type,
enum ECPGttype, char *, char *, long, long, long,
enum ARRAY_TYPE, enum COMPAT_MODE, bool);
That second comment is completely bogus. It's not this patch's fault,
it's been like that forever, but just happened to notice..
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done
Hi all,
In ecpg_add_mem of memory.c, we use ecpg_alloc but there is actually
no NULL-pointer check. If an OOM shows up exactly at this point, this
is likely to cause a crash. Attached patch adds some extra processing
to ecpg_add_mem to check if the allocation fails, and to fail properly
if an OOM appears.
This issue has been pointed out by Coverity, and I guessed the legwork
needed by myself.
Regards,
--
Michael
From 250349c31f86028284bb94f896916a0bb449d299 Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Tue, 3 Feb 2015 16:21:50 +0900
Subject: [PATCH] Fix unlikely-to-happen crash in ecpg_add_mem
This routine was called ecpg_alloc to allocate to memory but did not
actually check the returned pointer allocated, potentially NULL which
is actually the result of a malloc call.
Issue noted by Coverity, though I guessed the legwork needed here.
---
src/interfaces/ecpg/ecpglib/descriptor.c | 14 --
src/interfaces/ecpg/ecpglib/execute.c| 12 ++--
src/interfaces/ecpg/ecpglib/extern.h | 2 +-
src/interfaces/ecpg/ecpglib/memory.c | 9 -
4 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index b2990ca..bfa3e287 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -440,7 +440,12 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
return false;
}
*(void **) var = mem;
- ecpg_add_mem(mem, lineno);
+ if (!ecpg_add_mem(mem, lineno))
+ {
+ ecpg_free(mem);
+ va_end(args);
+ return false;
+ }
var = mem;
}
@@ -518,7 +523,12 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
return false;
}
*(void **) data_var.ind_pointer = mem;
- ecpg_add_mem(mem, lineno);
+ if (!ecpg_add_mem(mem, lineno))
+ {
+ecpg_free(mem);
+va_end(args);
+return false;
+ }
data_var.ind_value = mem;
}
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index abe60a5..912e75c 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -402,7 +402,11 @@ ecpg_store_result(const PGresult *results, int act_field,
if (!var-value)
return false;
*((char **) var-pointer) = var-value;
- ecpg_add_mem(var-value, stmt-lineno);
+ if (!ecpg_add_mem(var-value, stmt-lineno))
+ {
+ ecpg_free(var-value);
+ return false;
+ }
}
/* allocate indicator variable if needed */
@@ -414,7 +418,11 @@ ecpg_store_result(const PGresult *results, int act_field,
if (!var-ind_value)
return false;
*((char **) var-ind_pointer) = var-ind_value;
- ecpg_add_mem(var-ind_value, stmt-lineno);
+ if (!ecpg_add_mem(var-ind_value, stmt-lineno))
+ {
+ ecpg_free(var-ind_value);
+ return false;
+ }
}
/* fill the variable with the tuple(s) */
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index 3836007..3e93b53 100644
--- a/src/interfaces/ecpg/ecpglib/extern.h
+++ b/src/interfaces/ecpg/ecpglib/extern.h
@@ -137,7 +137,7 @@ extern struct var_list *ivlist;
/* Here are some methods used by the lib. */
/* Returns a pointer to a string containing a simple type name. */
-void ecpg_add_mem(void *ptr, int lineno);
+bool ecpg_add_mem(void *ptr, int lineno);
bool ecpg_get_data(const PGresult *, int, int, int, enum ECPGttype type,
enum ECPGttype, char *, char *, long, long, long,
diff --git a/src/interfaces/ecpg/ecpglib/memory.c b/src/interfaces/ecpg/ecpglib/memory.c
index a09cd26..1ffe3e1 100644
--- a/src/interfaces/ecpg/ecpglib/memory.c
+++ b/src/interfaces/ecpg/ecpglib/memory.c
@@ -104,14 +104,21 @@ static struct auto_mem *auto_allocs = NULL;
#define set_auto_allocs(am) do { auto_allocs = (am); } while(0)
#endif
-void
+bool
ecpg_add_mem(void *ptr, int lineno)
{
struct auto_mem *am = (struct auto_mem *) ecpg_alloc(sizeof(struct auto_mem), lineno);
+ if (!am)
+ {
+ ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
+ return false;
+ }
+
am-pointer = ptr;
am-next = get_auto_allocs();
set_auto_allocs(am);
+ return true;
}
void
--
2.2.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
