Re: [PATCH] FIx resource leaks (pg_resetwal.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Date
Msg-id CAEudQAoJDNKNANhqTiavh9Q=FRS7DdKaFhdGjk+ZRUGuO_yCyw@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] FIx resource leaks (pg_resetwal.c)  (Andres Freund <andres@anarazel.de>)
Responses Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Re: [PATCH] FIx resource leaks (pg_resetwal.c)
List pgsql-hackers
Em qui., 23 de abr. de 2020 às 15:27, Andres Freund <andres@anarazel.de> escreveu:
Hi,

On 2020-04-23 15:20:59 -0300, Ranier Vilela wrote:
> Per Coverity.
>
> read_controlfile alloc memory with pg_malloc and fail in releasing the
> memory.

Seriously, this is getting really ridiculous. You're posting badly
vetted, often nearly verbatim, coverity reports. Many of them are
obvious false positives. This is just producing noise.
I do not agree in any way. At the very least what I am reporting is suspect. And if I already propose a solution even if it is not the best, it is much better than being silent and missing the opportunity to fix a bug.
Ridiculous is your lack of education.
 

Please stop.
I will ignore.
 
> diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
> index 233441837f..673ab0204c 100644
> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -608,6 +608,7 @@ read_controlfile(void)
>       len = read(fd, buffer, PG_CONTROL_FILE_SIZE);
>       if (len < 0)
>       {
> +             pg_free(buffer);               
>               pg_log_error("could not read file \"%s\": %m", XLOG_CONTROL_FILE);
>               exit(1);
>       }

There's an exit() two lines later, this is obviously not necessary.
Excess.
 
Did you read patch all over?

  memcpy(&ControlFile, buffer, sizeof(ControlFile));
+ pg_free(buffer);
 
  /* return false if WAL segment size is not valid */
  if (!IsValidWalSegSize(ControlFile.xlog_seg_size))
@@ -644,6 +646,7 @@ read_controlfile(void)
 
  return true;
  }
+    pg_free(buffer);
 
  /* Looks like it's a mess. */
  pg_log_warning("pg_control exists but is broken or wrong version; ignoring it");

Report for Coverity:

*** CID 1425435:  Resource leaks  (RESOURCE_LEAK)
/dll/postgres/src/bin/pg_resetwal/pg_resetwal.c: 650 in read_controlfile()
644    
645              return true;
646      }
647    
648      /* Looks like it's a mess. */
649      pg_log_warning("pg_control exists but is broken or wrong version; ignoring it");
>>>     CID 1425435:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "buffer" going out of scope leaks the storage it points to.
650      return false;
651     }
652    
653    
654     /*
655      * Guess at pg_control values when we can't read

regards,
Ranier Vilela
Attachment

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Next
From: Robert Haas
Date:
Subject: Re: +(pg_lsn, int8) and -(pg_lsn, int8) operators