Thread: Collection of memory leaks for ECPG driver
Hi all, 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. Regards, -- Michael
Attachment
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
On Mon, Jun 8, 2015 at 9:22 PM, Michael Meskes wrote:
> On Mon, Jun 08, 2015 at 01:57:32PM +0900, Michael Paquier wrote:
>> 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.
And the caller needs to be sure that str actually allocates enough
space.. That's not directly ECPG's business, still it feels
uncomfortable. I fixed it with the attached.
>> 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?
The world is full of surprises, and even if free(NULL) is a noop on
modern platforms, I would rather take it defensively and check for a
NULL pointer as Postgres supports many platforms. Other code paths
tend to do already so, per se for example ECPGconnect.
>> 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
morethan a simple free() command?
Perhaps I am overdoing it. I would have them for correctness (some
other code paths do so) but if you think that the impact is minimal
there then I am fine to not modify descriptor.c.
> 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.
A new patch is attached.
Regards,
--
Michael
Attachment
On Mon, Jun 08, 2015 at 10:50:25PM +0900, Michael Paquier wrote: > And the caller needs to be sure that str actually allocates enough > space.. That's not directly ECPG's business, still it feels But there is no way for us to fix this as we want to implement the API as defined in Informix. > uncomfortable. I fixed it with the attached. Thanks, committed. > The world is full of surprises, and even if free(NULL) is a noop on > modern platforms, I would rather take it defensively and check for a > NULL pointer as Postgres supports many platforms. Other code paths > tend to do already so, per se for example ECPGconnect. Right, that's because they were developed before free received the safeguard, or the programmer simply didn't know at thatpoint in time. Unless I'm mistaken we have other code that only calls free() without an additional safeguard, so whyshuld ecpglib be special? If you don't like it using both approaches, feel free to remove the check in the other case.:) More seriously, though, does anyone know of any platform where free(NULL) is *not* a noop? I don't like adding stuff just because of the world being full of surprises. There may also be other functions not working as advertized. Wouldn't that then mean we cannot use any function nor provided by ourselves? > Perhaps I am overdoing it. I would have them for correctness (some > other code paths do so) but if you think that the impact is minimal > there then I am fine to not modify descriptor.c. Sorry, I wasn't referring to descriptor.c but to the whole preproc/ subtree. But as I already said, since we went throughthe effort we can of course apply it. Will be in my next push. Thanks. 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
On Fri, Jun 12, 2015 at 10:01 PM, Michael Meskes <meskes@postgresql.org> wrote: > On Mon, Jun 08, 2015 at 10:50:25PM +0900, Michael Paquier wrote: > Right, that's because they were developed before free received the safeguard, or the programmer simply didn't know at thatpoint in time. Unless I'm mistaken we have other code that only calls free() without an additional safeguard, so whyshuld ecpglib be special? If you don't like it using both approaches, feel free to remove the check in the other case.:) Well, I can send patches... > More seriously, though, does anyone know of any platform where free(NULL) is *not* a noop? I recall reading that some past versions of SunOS crashed, but it is rather old... -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jun 12, 2015 at 10:01 PM, Michael Meskes <meskes@postgresql.org> wrote:
>> More seriously, though, does anyone know of any platform where free(NULL) is *not* a noop?
> I recall reading that some past versions of SunOS crashed, but it is
> rather old...
Yeah, SunOS 4.x had issues here, but it's long dead. More to the point,
both C89 and Single Unix Spec v2 clearly say that free(NULL) is a no-op;
and it's been many years since we agreed that we had no interest in
supporting platforms that didn't meet at least those minimum standards.
So there is no need to worry about any code of ours that does free(NULL).
But having said that, I would not be in a hurry to remove any existing
if-guards for the case. For one thing, it makes the code look more
similar to backend code that uses palloc/pfree, where we do *not* allow
pfree(NULL). There's also the point that changing longstanding code
creates back-patching hazards, so unless there's a clear gain it's best
not to.
regards, tom lane
On Sat, Jun 13, 2015 at 12:02:40AM -0400, Tom Lane wrote: > But having said that, I would not be in a hurry to remove any existing > if-guards for the case. For one thing, it makes the code look more > similar to backend code that uses palloc/pfree, where we do *not* allow > pfree(NULL). There's also the point that changing longstanding code > creates back-patching hazards, so unless there's a clear gain it's best > not to. Makes sense, but there is no point in adding hos if-guards to old code that doesn't have it either,right? 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
On Sat, Jun 13, 2015 at 6:25 PM, Michael Meskes <meskes@postgresql.org> wrote: > On Sat, Jun 13, 2015 at 12:02:40AM -0400, Tom Lane wrote: >> But having said that, I would not be in a hurry to remove any existing >> if-guards for the case. For one thing, it makes the code look more >> similar to backend code that uses palloc/pfree, where we do *not* allow >> pfree(NULL). There's also the point that changing longstanding code >> creates back-patching hazards, so unless there's a clear gain it's best >> not to. > > Makes sense, but there is no point in adding hos if-guards to old code that > doesn't have it either,right? In any case, whatever the final decision done here, I just wanted to point out that there is still a leak in connect.c. Per se the attached patch, that does not check for a NULL pointer before ecpg_free because other code paths in the routine patched don't do so. So you get something locally consistent ;) -- Michael
Attachment
On Sun, Jun 14, 2015 at 08:43:13PM +0900, Michael Paquier wrote: > point out that there is still a leak in connect.c. Per se the attached > patch, that does not check for a NULL pointer before ecpg_free because > other code paths in the routine patched don't do so. So you get > something locally consistent ;) Thanks, 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
On Mon, Jun 15, 2015 at 9:33 PM, Michael Meskes <meskes@postgresql.org> wrote: > On Sun, Jun 14, 2015 at 08:43:13PM +0900, Michael Paquier wrote: >> point out that there is still a leak in connect.c. Per se the attached >> patch, that does not check for a NULL pointer before ecpg_free because >> other code paths in the routine patched don't do so. So you get >> something locally consistent ;) > > Thanks, committed. Thanks. -- Michael