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. */
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,
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

On Tue, Feb 3, 2015 at 7:50 PM, Heikki Linnakangas
<hlinnakangas@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

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