Re: Collection of memory leaks for ECPG driver

From: Michael Meskes <meskes(at)postgresql(dot)org>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Collection of memory leaks for ECPG driver
Date: 2015-06-08 12:22:27
Message-ID: 20150608122227.GA26885@feivel.credativ.lan
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Jun 08, 2015 at 01:57:32PM +0900, Michael Paquier wrote:
> Please find attached a patch aimed at fixing a couple of memory leaks
> in the ECPG driver. Coverity (and sometimes I after reading some other
> code paths) found them.

And some are not correct it seems. But then some of the code isn't either. :)

> diff --git a/src/interfaces/ecpg/compatlib/informix.c b/src/interfaces/ecpg/compatlib/informix.c
> index d6de3ea..c1e3dfb 100644
> --- a/src/interfaces/ecpg/compatlib/informix.c
> +++ b/src/interfaces/ecpg/compatlib/informix.c
> @@ -672,6 +672,7 @@ intoasc(interval * i, char *str)
> if (!str)
> return -errno;
>
> + free(str);
> return 0;
> }

This function never worked it seems, we need to use a temp string, copy it to str and then free the temp one.

> diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
> index 55c5680..12f445d 100644
> --- a/src/interfaces/ecpg/ecpglib/connect.c
> +++ b/src/interfaces/ecpg/ecpglib/connect.c
> @@ -298,7 +298,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
> envname = getenv("PG_DBPATH");
> if (envname)
> {
> - ecpg_free(dbname);
> + if (dbname)
> + ecpg_free(dbname);
> dbname = ecpg_strdup(envname, lineno);
> }

This is superfluous, at least with glibc. free()'s manpage clearly says "If
ptr is NULL, no operation is performed.", so why should we check again? Or do
we have architectures where free(NULL) is not a noop?

> diff --git a/src/interfaces/ecpg/preproc/descriptor.c b/src/interfaces/ecpg/preproc/descriptor.c
> index 053a7af..ebd95d3 100644
> --- a/src/interfaces/ecpg/preproc/descriptor.c
> +++ b/src/interfaces/ecpg/preproc/descriptor.c

Do we really have to worry about these in a short running application like a precompiler, particularly if they add more than a simple free() command?

But then, you already did the work, so why not. Anyway, please tell me if you want to update the patch or if I should commit whatever is clear already.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Gould 2015-06-08 12:59:50 Re: [CORE] Restore-reliability mode
Previous Message Dean Rasheed 2015-06-08 10:02:32 Re: CREATE POLICY and RETURNING