Re: Collection of memory leaks for ECPG driver - Mailing list pgsql-hackers

From Michael Meskes
Subject Re: Collection of memory leaks for ECPG driver
Date
Msg-id 20150608122227.GA26885@feivel.credativ.lan
Whole thread Raw
In response to Collection of memory leaks for ECPG driver  (Michael Paquier <michael.paquier@gmail.com>)
Responses Re: Collection of memory leaks for ECPG driver
List 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
thana 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
commitwhatever 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



pgsql-hackers by date:

Previous
From: Dean Rasheed
Date:
Subject: Re: CREATE POLICY and RETURNING
Next
From: David Gould
Date:
Subject: Re: [CORE] Restore-reliability mode