Обсуждение: BUG #15838: [contrib] vacuumlo: schema variable checked for NULL three times

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

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

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>:
> 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