Lists: | pgsql-hackers |
---|
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done |
Date: | 2015-02-03 07:28:10 |
Message-ID: | CAB7nPqSnZMiRMZ_HJ2XLm2i9jcs3SPRJ-8hBmG0f24OjHcyc+A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
20150203_ecpg_fix_dereferenced.patch | application/x-patch | 3.8 KB |
From: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done |
Date: | 2015-02-03 10:50:23 |
Message-ID: | 54D0A7EF.6080102@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done |
Date: | 2015-02-03 13:44:17 |
Message-ID: | CAB7nPqS-oWUbRjPwzFm9WtEK0j-6pVvHT=hTErBh56tDdn=-rQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Tue, Feb 3, 2015 at 7:50 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)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
Attachment | Content-Type | Size |
---|---|---|
20150203_ecpg_fix_dereferenced_v2.patch | application/x-patch | 4.9 KB |
From: | Michael Meskes <meskes(at)postgresql(dot)org> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done |
Date: | 2015-02-05 14:17:13 |
Message-ID: | 20150205141713.GA16113@feivel.credativ.lan |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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