BUG #15838: [contrib] vacuumlo: schema variable checked for NULL three times

Started by PG Bug reporting formalmost 7 years ago3 messagesbugs
Jump to latest
#1PG Bug reporting form
noreply@postgresql.org

The following bug has been logged on the website:

Bug reference: 15838
Logged by: Timur Birsh
Email address: taem@linukz.org
PostgreSQL version: 11.2
Operating system: CentOS 7
Description:

Hello,

vacuumlo() has this (starting on line 239):

if (!schema || !table || !field)
{
fprintf(stderr, "%s", PQerrorMessage(conn));
PQclear(res);
PQfinish(conn);
if (schema != NULL)
PQfreemem(schema);
if (schema != NULL)
PQfreemem(table);
if (schema != NULL)
PQfreemem(field);
return -1;
}

I think this can be replaced with this:

if (!schema || !table || !field)
{
fprintf(stderr, "%s", PQerrorMessage(conn));
PQclear(res);
PQfinish(conn);
if (schema != NULL) {
PQfreemem(schema);
PQfreemem(table);
PQfreemem(field);
}
return -1;
}

Thanks,
Timur

#2Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: PG Bug reporting form (#1)
Re: BUG #15838: [contrib] vacuumlo: schema variable checked for NULL three times

On 07/06/2019 09:15, PG Bug reporting form wrote:

vacuumlo() has this (starting on line 239):

if (!schema || !table || !field)
{
fprintf(stderr, "%s", PQerrorMessage(conn));
PQclear(res);
PQfinish(conn);
if (schema != NULL)
PQfreemem(schema);
if (schema != NULL)
PQfreemem(table);
if (schema != NULL)
PQfreemem(field);
return -1;
}

I think this can be replaced with this:

if (!schema || !table || !field)
{
fprintf(stderr, "%s", PQerrorMessage(conn));
PQclear(res);
PQfinish(conn);
if (schema != NULL) {
PQfreemem(schema);
PQfreemem(table);
PQfreemem(field);
}
return -1;
}

Hmm. Currently, if allocating 'schema' fails, but allocating 'table' or
'field' succeeds, you leak memory. I'm pretty sure that was intended to be:

if (!schema || !table || !field)
{
fprintf(stderr, "%s", PQerrorMessage(conn));
PQclear(res);
PQfinish(conn);
if (schema != NULL)
PQfreemem(schema);
if (table != NULL)
PQfreemem(table);
if (field != NULL)
PQfreemem(field);
return -1;
}

I'll go fix it that way, thanks for the report!

- Heikki

#3Timur Birsh
taem@linukz.org
In reply to: Heikki Linnakangas (#2)
Re: BUG #15838: [contrib] vacuumlo: schema variable checked for NULL three times

Hello Heikki,

Thanks for your reply.

Yes, my first intention was to report that 'table' and 'field' should be checked also, but I was not sure.

Regards.

07.06.2019, 15:21, "Heikki Linnakangas" <hlinnaka@iki.fi>:

Show quoted text

On 07/06/2019 09:15, PG Bug reporting form wrote:

 vacuumlo() has this (starting on line 239):

                  if (!schema || !table || !field)
                  {
                          fprintf(stderr, "%s", PQerrorMessage(conn));
                          PQclear(res);
                          PQfinish(conn);
                          if (schema != NULL)
                                  PQfreemem(schema);
                          if (schema != NULL)
                                  PQfreemem(table);
                          if (schema != NULL)
                                  PQfreemem(field);
                          return -1;
                  }

 I think this can be replaced with this:

                  if (!schema || !table || !field)
                  {
                          fprintf(stderr, "%s", PQerrorMessage(conn));
                          PQclear(res);
                          PQfinish(conn);
                          if (schema != NULL) {
                                  PQfreemem(schema);
                                  PQfreemem(table);
                                  PQfreemem(field);
                          }
                          return -1;
                  }

Hmm. Currently, if allocating 'schema' fails, but allocating 'table' or
'field' succeeds, you leak memory. I'm pretty sure that was intended to be:

         if (!schema || !table || !field)
         {
             fprintf(stderr, "%s", PQerrorMessage(conn));
             PQclear(res);
             PQfinish(conn);
             if (schema != NULL)
                 PQfreemem(schema);
             if (table != NULL)
                 PQfreemem(table);
             if (field != NULL)
                 PQfreemem(field);
             return -1;
         }

I'll go fix it that way, thanks for the report!

- Heikki