[PATCH] FIx resource leaks (pg_resetwal.c)

Started by Ranier Vilelaover 5 years ago7 messages

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

1 attachment(s)

Hi,
Per Coverity.

read_controlfile alloc memory with pg_malloc and fail in releasing the
memory.

regards,
Ranier Vilela

Attachments:

fix_resource_leaks_pg_resetwal.patchapplication/octet-stream; name=fix_resource_leaks_pg_resetwal.patchDownload

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);
 	}
@@ -631,6 +632,7 @@ read_controlfile(void)
 		}
 
 		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");

Andres Freund

andres@anarazel.de

over 5 years ago

In reply to: Ranier Vilela (#1)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

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.

Please stop.

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.

Greetings,

Andres Freund

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

In reply to: Andres Freund (#2)

1 attachment(s)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

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

Attachments:

fix_resource_leaks_pg_resetwal_v2.patchapplication/octet-stream; name=fix_resource_leaks_pg_resetwal_v2.patchDownload

diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 233441837f..fd6784622c 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -631,6 +631,7 @@ read_controlfile(void)
 		}
 
 		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 +645,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");

Peter Geoghegan

pg@bowt.ie

over 5 years ago

In reply to: Ranier Vilela (#3)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

On Thu, Apr 23, 2020 at 11:41 AM Ranier Vilela <ranier.vf@gmail.com> wrote:

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.

The problem with that theory is that you're not creating any value
over simply running Coverity directly. Your patches don't seem to be
based on any real analysis beyond what makes Coverity stop
complaining, which is not helpful.

For example, the nbtree.c/btvacuumpage() issue you reported yesterday
involved a NULL pointer dereference, but if the code path in question
ever dereferenced the NULL pointer then it would be fundamentally
wrong in many other ways, probably leading to data corruption. The fix
that you posted obviously completely missed the point. Even when
Coverity identifies a serious issue, it usually needs to be carefully
interpreted.

Anybody can run Coverity. Many of us do. Maybe the approach you've
taken would have had a noticeable benefit if you were not dealing with
a codebase that has already been subject to lots of triage of Coverity
issues. But that's not the case.

Ridiculous is your lack of education.

This isn't helping you at all.

--
Peter Geoghegan

Robert Haas

robertmhaas@gmail.com

over 5 years ago

In reply to: Ranier Vilela (#3)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

On Thu, Apr 23, 2020 at 2:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

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.

That's rather rude. I doubt that you know anything about how much
education Andres does nor does not have. The fact that he doesn't
agree with you does not mean that he is poorly educated.

On the substance of the issue, I see from the commit log that you've
gotten a few real issues fixed -- but I also agree with Andres that
you've reported a lot of things that are not real issues, and that
takes up other people's time looking at things that really don't
matter. Please make an effort not to report things that don't actually
need to be fixed.

pg_resetwal exits very quickly, generally in a small fraction of a
second. The allocation you're at pains to free only happens once per
execution and allocates only 8kB. Trying to free allocations that are
tiny and short-lived has no benefit. It's better to let the program
exit that much quicker, at which point all the memory is freed anyway.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

In reply to: Peter Geoghegan (#4)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

Em qui., 23 de abr. de 2020 às 16:27, Peter Geoghegan <pg@bowt.ie> escreveu:

On Thu, Apr 23, 2020 at 11:41 AM Ranier Vilela <ranier.vf@gmail.com>
wrote:

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.

The problem with that theory is that you're not creating any value
over simply running Coverity directly. Your patches don't seem to be
based on any real analysis beyond what makes Coverity stop
complaining, which is not helpful.

In some cases, this may be true. But in fact I already fixed some bugs with
this technique. Even you already used a patch of mine to provide a fix.
Wasn't that helpful?
1.
/messages/by-id/CAEudQAqJ=MVqJd4MHi=iMLismngE4GJqdiEZa1isxF3Pem-udg@mail.gmail.com

For example, the nbtree.c/btvacuumpage() issue you reported yesterday
involved a NULL pointer dereference, but if the code path in question
ever dereferenced the NULL pointer then it would be fundamentally
wrong in many other ways, probably leading to data corruption. The fix
that you posted obviously completely missed the point. Even when
Coverity identifies a serious issue, it usually needs to be carefully
interpreted.

I disagree. In case of nbtree.c/btvacuumpag().
If you are validating "opaque" pointer, in three different ways to proceed
with cleaning, nothing more correct than validating the most important
first, if the pointer is really valid. And that is what the patch does.

Anybody can run Coverity. Many of us do. Maybe the approach you've
taken would have had a noticeable benefit if you were not dealing with
a codebase that has already been subject to lots of triage of Coverity
issues.

Sorry, but the plsql-bugs list, has many reports of segmentation faults,
that shouldn't exist, if everyone uses Coverity or other tools, after
writing the code.

Ridiculous is your lack of education.

This isn't helping you at all.

Consideration and respect first.

regards,
Ranier Vilela

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

In reply to: Robert Haas (#5)

Re: [PATCH] FIx resource leaks (pg_resetwal.c)

Em qui., 23 de abr. de 2020 às 16:43, Robert Haas <robertmhaas@gmail.com>
escreveu:

On Thu, Apr 23, 2020 at 2:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

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.

That's rather rude. I doubt that you know anything about how much
education Andres does nor does not have. The fact that he doesn't
agree with you does not mean that he is poorly educated.

Sorry Robert.

On the substance of the issue, I see from the commit log that you've
gotten a few real issues fixed -- but I also agree with Andres that
you've reported a lot of things that are not real issues, and that
takes up other people's time looking at things that really don't
matter. Please make an effort not to report things that don't actually
need to be fixed.

All my patches don't just leave my head. It comes from reports of analysis
tools, by themselves, they are already suspect.
I confess that FATAL error log, confused me a lot and since then, I have
tried my best not to make the same mistakes.

pg_resetwal exits very quickly, generally in a small fraction of a
second. The allocation you're at pains to free only happens once per
execution and allocates only 8kB. Trying to free allocations that are
tiny and short-lived has no benefit. It's better to let the program
exit that much quicker, at which point all the memory is freed anyway.

Read_controlfile is a function, as it stands, it is useless to be reused.

best regards,
Ranier Vilela