Re: Collection of memory leaks for ECPG driver

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