Thread: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Bill Parker

Date:

11 June 2015, 22:31:50

============================================================================
                        POSTGRESQL BUG REPORT TEMPLATE
============================================================================

Your name               : Bill Parker
Your email address      : wp02855 at gmail dot com

System Configuration:
---------------------
Architecture (example: Intel Pentium)         : x86/x86-64/AMD

Operating System (example: Linux 2.4.18)      : Linux 3.11.6-4

PostgreSQL version (example: PostgreSQL 9.4.3): PostgreSQL 9.4.x

Compiler used (example: gcc 3.3.5)            : gcc version 4.8.1

Please enter a FULL description of your problem:
------------------------------------------------

Hello All,

   In reviewing some code, in directory 'postgresql-9.4.3/src/interfaces/ecpg/ecpglib',
file 'misc.c', there are several instances where a call to malloc()
is made, but no check for a return value of NULL is made, which
would indicate failure.   Additionally, if sqlca = malloc() fails,
ecpg_init_sqlca would be called with variable 'sqlca' equal to NULL?

If you know how this problem might be fixed, list the solution below:
---------------------------------------------------------------------

The patch file below addresses these issues:

--- misc.c.orig 2015-06-11 09:23:13.807020490 -0700
+++ misc.c      2015-06-11 09:32:10.077177669 -0700
@@ -143,6 +143,9 @@
        if (sqlca == NULL)
        {
                sqlca = malloc(sizeof(struct sqlca_t));
+               if (sqlca == NULL) {    /* malloc() failed, now what should we do? */
+                   ecpg_log("Unable to allocate memory in ECPGget_sqlca()\n");
+               }
                ecpg_init_sqlca(sqlca);
                pthread_setspecific(sqlca_key, sqlca);
        }

Please feel free to review and comment on the above patch file...

I am attaching the patch file to this bug report

Bill Parker (wp02855 at gmail dot com)

Attachment

misc.c.patch

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Paquier

Date:

12 June 2015, 09:53:47

(Adding Michael Meskes in CC:)

On Fri, Jun 12, 2015 at 4:11 AM, Bill Parker <wp02855@gmail.com> wrote:
> --- misc.c.orig 2015-06-11 09:23:13.807020490 -0700
> +++ misc.c      2015-06-11 09:32:10.077177669 -0700
> @@ -143,6 +143,9 @@
>         if (sqlca == NULL)
>         {
>                 sqlca = malloc(sizeof(struct sqlca_t));
> +               if (sqlca == NULL) {    /*  malloc() failed, now what should
> we do? */
> +                   ecpg_log("Unable to allocate memory in
> ECPGget_sqlca()\n");
> +               }
>                 ecpg_init_sqlca(sqlca);
>                 pthread_setspecific(sqlca_key, sqlca);
>         }
>
> Please feel free to review and comment on the above patch file...
>
> I am attaching the patch file to this bug report
>
> Bill Parker (wp02855 at gmail dot com)

Nice catch. Regarding your patch, it seems to me that this is not
enough. I think that we should return NULL with ECPGget_sqlca in case
of an OOM instead of logging a message with ecpg_log and let each code
path decide what to do when sqlca is NULL. Some code paths can
directly use ecpg_raise with ECPG_OUT_OF_MEMORY. Other code paths,
like the ones in error.c will need to show up with appropriate error
messages.
--
Michael

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Meskes

Date:

12 June 2015, 16:06:12

On Thu, Jun 11, 2015 at 12:11:37PM -0700, Bill Parker wrote:
> file 'misc.c', there are several instances where a call to malloc()
> is made, but no check for a return value of NULL is made, which

Your patch lists one such case, but where are the other "several"?

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.meskes at gmail dot com
VfL Borussia! ForÃ§a BarÃ§a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Meskes

Date:

12 June 2015, 16:06:42

On Fri, Jun 12, 2015 at 03:53:43PM +0900, Michael Paquier wrote:
> (Adding Michael Meskes in CC:)

Thanks.

> Nice catch. Regarding your patch, it seems to me that this is not
> enough. I think that we should return NULL with ECPGget_sqlca in case
> of an OOM instead of logging a message with ecpg_log and let each code
> path decide what to do when sqlca is NULL. Some code paths can
> directly use ecpg_raise with ECPG_OUT_OF_MEMORY. Other code paths,
> like the ones in error.c will need to show up with appropriate error
> messages.

Agreed on all accounts.

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.meskes at gmail dot com
VfL Borussia! ForÃ§a BarÃ§a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Paquier

Date:

15 June 2015, 10:35:58

On Fri, Jun 12, 2015 at 10:06 PM, Michael Meskes <meskes@postgresql.org> wrote:
> On Fri, Jun 12, 2015 at 03:53:43PM +0900, Michael Paquier wrote:
>> (Adding Michael Meskes in CC:)
>
> Thanks.
>
>> Nice catch. Regarding your patch, it seems to me that this is not
>> enough. I think that we should return NULL with ECPGget_sqlca in case
>> of an OOM instead of logging a message with ecpg_log and let each code
>> path decide what to do when sqlca is NULL. Some code paths can
>> directly use ecpg_raise with ECPG_OUT_OF_MEMORY. Other code paths,
>> like the ones in error.c will need to show up with appropriate error
>> messages.
>
> Agreed on all accounts.

So, here is a patch implementing those ideas. In code paths where a
line number is available ecpg_raise() is called to report the error.
In other paths ecpg_log() is used to log an "out of memory" message.
Now, the routines of error.c, like ecpg_raise() can fail as well their
malloc() call, hence it seems adapted to me to fallback to ecpg_log()
and report the error to the user.
Thoughts?
--
Michael

Attachment

20150615_ecpg_fix_malloc.patch

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Meskes

Date:

15 June 2015, 15:34:27

On Mon, Jun 15, 2015 at 04:35:47PM +0900, Michael Paquier wrote:
> So, here is a patch implementing those ideas. In code paths where a
> line number is available ecpg_raise() is called to report the error.
> In other paths ecpg_log() is used to log an "out of memory" message.
> Now, the routines of error.c, like ecpg_raise() can fail as well their
> malloc() call, hence it seems adapted to me to fallback to ecpg_log()
> and report the error to the user.
> Thoughts?

Sounds good to me. Patch committed.

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.meskes at gmail dot com
VfL Borussia! ForÃ§a BarÃ§a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: Lack of Sanity Checking in file 'misc.c' for PostgreSQL 9.4.x

From

Michael Paquier

Date:

16 June 2015, 03:06:18

On Mon, Jun 15, 2015 at 9:34 PM, Michael Meskes <meskes@postgresql.org> wrote:
> On Mon, Jun 15, 2015 at 04:35:47PM +0900, Michael Paquier wrote:
>> So, here is a patch implementing those ideas. In code paths where a
>> line number is available ecpg_raise() is called to report the error.
>> In other paths ecpg_log() is used to log an "out of memory" message.
>> Now, the routines of error.c, like ecpg_raise() can fail as well their
>> malloc() call, hence it seems adapted to me to fallback to ecpg_log()
>> and report the error to the user.
>> Thoughts?
>
> Sounds good to me. Patch committed.

Thanks.
--
Michael