[PATCH] ecpg: fix progname memory leak
Hi,
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
Best regards,
Maksim Kita
Attachments:
0001-ecpg-fix-progname-memory-leak.patchtext/x-diff; charset=us-asciiDownload
From 2f730117cbff1207c6b0bffc3097831fe5028fec Mon Sep 17 00:00:00 2001
From: Maksim Kita <kitaetoya@gmail.com>
Date: Wed, 7 Oct 2020 14:15:52 +0300
Subject: [PATCH] ecpg: fix progname memory leak
Added goto on error cleanup for progname in ecpg main.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
---
src/interfaces/ecpg/preproc/ecpg.c | 33 ++++++++++++++------
src/interfaces/ecpg/preproc/preproc_extern.h | 1 +
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 44a6d5119b..1be9483d88 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -127,7 +127,7 @@ main(int argc, char *const argv[])
bool verbose = false,
header_mode = false;
struct _include_path *ip;
- const char *progname;
+ const char *progname = NULL;
char my_exec_path[MAXPGPATH];
char include_path[MAXPGPATH];
@@ -138,7 +138,8 @@ main(int argc, char *const argv[])
if (find_my_exec(argv[0], my_exec_path) < 0)
{
fprintf(stderr, _("%s: could not locate my own executable path\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
if (argc > 1)
@@ -146,12 +147,14 @@ main(int argc, char *const argv[])
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
help(progname);
- exit(0);
+ ret_value = EXIT_OK;
+ goto err;
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
{
printf("ecpg (PostgreSQL) %s\n", PG_VERSION);
- exit(0);
+ ret_value = EXIT_OK;
+ goto err;
}
}
@@ -216,7 +219,8 @@ main(int argc, char *const argv[])
else
{
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
break;
case 'r':
@@ -229,7 +233,8 @@ main(int argc, char *const argv[])
else
{
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
break;
case 'D':
@@ -245,7 +250,8 @@ main(int argc, char *const argv[])
break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
}
@@ -264,14 +270,16 @@ main(int argc, char *const argv[])
for (ip = include_paths; ip != NULL; ip = ip->next)
fprintf(stderr, " %s\n", ip->path);
fprintf(stderr, _("end of search list\n"));
- return 0;
+ ret_value = EXIT_OK;
+ goto err;
}
if (optind >= argc) /* no files specified */
{
fprintf(stderr, _("%s: no input files specified\n"), progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
- return ILLEGAL_OPTION;
+ ret_value = ILLEGAL_OPTION;
+ goto err;
}
else
{
@@ -473,7 +481,7 @@ main(int argc, char *const argv[])
/*
* If there was an error, delete the output file.
*/
- if (ret_value != 0)
+ if (ret_value != EXIT_OK)
{
if (strcmp(output_filename, "-") != 0 && unlink(output_filename) != 0)
fprintf(stderr, _("could not remove output file \"%s\"\n"), output_filename);
@@ -489,5 +497,10 @@ main(int argc, char *const argv[])
free(input_filename);
}
}
+
+err:
+ if (progname)
+ free((void*)(progname));
+
return ret_value;
}
diff --git a/src/interfaces/ecpg/preproc/preproc_extern.h b/src/interfaces/ecpg/preproc/preproc_extern.h
index 51d5f94f07..5728dd8a96 100644
--- a/src/interfaces/ecpg/preproc/preproc_extern.h
+++ b/src/interfaces/ecpg/preproc/preproc_extern.h
@@ -106,6 +106,7 @@ extern int filtered_base_yylex(void);
/* return codes */
+#define EXIT_OK 0
#define ILLEGAL_OPTION 1
#define NO_INCLUDE_FILE 2
#define PARSE_ERROR 3
--
2.25.1
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
FWIW, I don't see much point in doing that. For one, we have a
more-or-less established rule that progname remains set until the
application leaves, and there are much more places where we leak
memory like that. As one example, just see the various places where
we use pg_strdup for option parsing. At the end, it does not really
matter as these are applications running for a short amount of time.
--
Michael
On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.FWIW, I don't see much point in doing that. For one, we have a
more-or-less established rule that progname remains set until the
application leaves, and there are much more places where we leak
memory like that. As one example, just see the various places where
we use pg_strdup for option parsing. At the end, it does not really
matter as these are applications running for a short amount of time.
Agreed, but what does the TODO item mean then?
Fix small memory leaks in ecpg
Memory leaks in a short running application like ecpg are not really
a problem, but make debugging more complicated
Should it be removed?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote:
On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.FWIW, I don't see much point in doing that. For one, we have a
more-or-less established rule that progname remains set until the
application leaves, and there are much more places where we leak
memory like that. As one example, just see the various places
where
we use pg_strdup for option parsing. At the end, it does not
really
matter as these are applications running for a short amount of
time.Agreed, but what does the TODO item mean then?
Fix small memory leaks in ecpg
Memory leaks in a short running application like ecpg are
not really
a problem, but make debugging more complicatedShould it be removed?
I'd say yes, let's remove it. Actually I wasn't even aware it's on
there. While I agree that it makes debugging of memory handling in ecpg
more difficult, I don't see much of a point in it.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De
Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
On Thu, Oct 8, 2020 at 06:58:14PM +0200, Michael Meskes wrote:
On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote:
On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.FWIW, I don't see much point in doing that. For one, we have a
more-or-less established rule that progname remains set until the
application leaves, and there are much more places where we leak
memory like that. As one example, just see the various places
where
we use pg_strdup for option parsing. At the end, it does not
really
matter as these are applications running for a short amount of
time.Agreed, but what does the TODO item mean then?
Fix small memory leaks in ecpg
Memory leaks in a short running application like ecpg are
not really
a problem, but make debugging more complicatedShould it be removed?
I'd say yes, let's remove it. Actually I wasn't even aware it's on
there. While I agree that it makes debugging of memory handling in ecpg
more difficult, I don't see much of a point in it.
OK, TODO removed.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Oct 7, 2020 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.FWIW, I don't see much point in doing that.
I hope that everyone takes 10 seconds and realizes that this appears to be
this person's first submitted patch. I would think a little more respect
for the attempt to patch a minor issue would be afforded to such a person.
Seems technically sound and they are not trying to change the world with
their first attempt.
Maybe a reminder that the TODO list is not always spectacular and that a
double check with the list before doing something might be in order (in
fact adding that to the top of the TODO list might be a great option here
as well).
It's not going to win a Turing award - but I thought this project was a
little more friendly then what I've seen in this thread towards a first
time contributor.
John
On Thu, Oct 8, 2020 at 10:13:53AM -0700, John W Higgins wrote:
On Wed, Oct 7, 2020 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
Fix progname memory leak in ecpg client.
Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.FWIW, I don't see much point in doing that.��
I hope that everyone takes 10 seconds and realizes that this appears to be this
person's first submitted patch. I would think a little more respect for the
attempt to patch a minor issue would be afforded to such a person. Seems
technically sound and they are not trying to change the world with their first
attempt.�Maybe a reminder that the TODO list is not always spectacular and that a double
check with the list before doing something might be in order (in fact adding
that to the top of the TODO list might be a great option here as well).It's not going to win a Turing award - but I thought this project was a little
more friendly then what I've seen in this thread towards a first time
contributor.
You mean like the warning at the top of the TODO list?
https://wiki.postgresql.org/wiki/Todo#Development_Process
WARNING for Developers: Unfortunately this list does not contain all the
information necessary for someone to start coding a feature. Some of
these items might have become unnecessary since they were added ---
others might be desirable but the implementation might be unclear. When
selecting items listed below, be prepared to first discuss the value of
the feature. Do not assume that you can select one, code it and then
expect it to be committed. Always discuss design on Hackers list before
starting to code. The flow should be:
Desirability -> Design -> Implement -> Test -> Review -> Commit
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Michael Meskes <meskes@postgresql.org> writes:
On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote:
Agreed, but what does the TODO item mean then?
Fix small memory leaks in ecpg
Memory leaks in a short running application like ecpg are not really
a problem, but make debugging more complicatedShould it be removed?
I'd say yes, let's remove it. Actually I wasn't even aware it's on
there. While I agree that it makes debugging of memory handling in ecpg
more difficult, I don't see much of a point in it.
Agreed. In theory there might be some point in removing leaks that happen
per-statement, so as to avoid unreasonable memory bloat when processing an
extremely long input file. In practice nobody has complained about that,
and if somebody did I'd probably question the sanity of putting so much
code into one file. (The C compiler would likely bloat even more while
processing the output...) Moreover, given the way the ecpg grammar works,
it'd be really painful to avoid all such leaks, and it might well
introduce bugs not fix them.
In any case, if this TODO item is going to lead to ideas as dubious
as "let's free progname before exiting", it's not helpful.
regards, tom lane
On Thu, Oct 8, 2020 at 10:13:53AM -0700, John W Higgins wrote:
It's not going to win a Turing award - but I thought this project was a
little more friendly then what I've seen in this thread towards a first
time contributor.
Instead, it is unfriendly.
It takes a lot of motivation to "try" to submit a patch.
Good lucky Maksim Kita.
Thanks for the support John
regards,
Ranier Vilela
Import Notes
Resolved by subject fallback
On Thu, Oct 08, 2020 at 01:17:25PM -0400, Tom Lane wrote:
Agreed. In theory there might be some point in removing leaks that happen
per-statement, so as to avoid unreasonable memory bloat when processing an
extremely long input file. In practice nobody has complained about that,
and if somebody did I'd probably question the sanity of putting so much
code into one file. (The C compiler would likely bloat even more while
processing the output...) Moreover, given the way the ecpg grammar works,
it'd be really painful to avoid all such leaks, and it might well
introduce bugs not fix them.
I got to wonder about that, and I don't quite see how to make all the
memory handling clean without having at least a concept of
per-statement memory context to make any cleanup easy once a statement
is done. That's a lot of infrastructure for a case nobody really
complained about, indeed..
In any case, if this TODO item is going to lead to ideas as dubious
as "let's free progname before exiting", it's not helpful.
+1. Agreed to just remove it.
--
Michael
On 8 Oct 2020, at 19:08, Bruce Momjian <bruce@momjian.us> wrote:
OK, TODO removed.
Potentially controversial proposal: Should we perhaps remove (for some value
of) the TODO list altogether?
The value of the list is questionable as it's not actually a TODO list for
established developers, and for aspiring new developers it's unlikely to give
good ideas for where to start (I know there's a big warning but not everyone
will read everything, the page is quite long).
Now, I don't actually suggest we *remove* it, as there is valuable curated
content, but that we rename it to something which won't encourage newcomers to
pick something from the list simply because it exists. The name "TODO" implies
that the list is something which it isn't.
Thoughts?
cheers ./daniel
On Fri, Oct 9, 2020 at 4:47 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Now, I don't actually suggest we *remove* it, as there is valuable curated
content, but that we rename it to something which won't encourage
newcomers to
pick something from the list simply because it exists. The name "TODO"
implies
that the list is something which it isn't.
+1
The name is very much at odds with the content. To pick one baffling
example, there's an entry under Free Space Map that dates to before the
on-disk FSM was invented. A more accurate, if not charitable, description
is "archive of stalled discussions". This is just a side effect of
non-controversial todos getting finished over time. That could be helped
with a round of aggressive culling.
Also, it's organized by functionality, which makes sense in a way, but I
don't find very useful in this context. Better categories might be
help-wanted,
good-first-issue (I know this is a tough one),
feature-request,
won't-fix (The memory leaks under discussion go here, it seems),
code-cleanup,
research-project,
documentation,
performance,
user-experience,
sql-standard
etc.
I suspect we will eventually want something like a full-blown issue
tracker, although I gather that's been rejected previously. But a wiki
page is not really suited for this.
I've found that a better source of actual "todo"s from developers can be
found in some commit messages, by grepping for "left for future work" or
"for another day". I've gotten patch ideas (and more than one committed)
from these. If I were asked, that's where I'd point aspiring developers.
--
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Oct 9, 2020 at 1:08 PM John Naylor <john.naylor@enterprisedb.com>
wrote:
On Fri, Oct 9, 2020 at 4:47 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Now, I don't actually suggest we *remove* it, as there is valuable curated
content, but that we rename it to something which won't encourage
newcomers to
pick something from the list simply because it exists. The name "TODO"
implies
that the list is something which it isn't.+1
+1 as well. The way things are today, that is a terrible name, and has been
for a double-digit number of years.
The name is very much at odds with the content. To pick one baffling
example, there's an entry under Free Space Map that dates to before the
on-disk FSM was invented. A more accurate, if not charitable, description
is "archive of stalled discussions". This is just a side effect of
non-controversial todos getting finished over time. That could be helped
with a round of aggressive culling.
That is one very good example indeed.
Also, it's organized by functionality, which makes sense in a way, but I
don't find very useful in this context. Better categories might be
help-wanted,
good-first-issue (I know this is a tough one),
feature-request,
won't-fix (The memory leaks under discussion go here, it seems),
code-cleanup,
research-project,
documentation,
performance,
user-experience,
sql-standard
etc.I suspect we will eventually want something like a full-blown issue
tracker, although I gather that's been rejected previously. But a wiki
page is not really suited for this.
Talk about "possibly controversial proposal" eh?
That said, having this in a different format would in no way fix the fact
that the information is mislabeled and obsolete. It would likely be
equally mislabeled and obsolete in an issue tracker. Just look at almost
any of the other projects out there that *do* use an issue tracker and have
been around for 20+ years.
That said, I am a believer in that we should have one, yes. But I am
equally certain that it would not help with *this* particular problem.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Magnus Hagander <magnus@hagander.net> writes:
That said, having this in a different format would in no way fix the fact
that the information is mislabeled and obsolete. It would likely be
equally mislabeled and obsolete in an issue tracker. Just look at almost
any of the other projects out there that *do* use an issue tracker and have
been around for 20+ years.
The long and the short of it is that a list like this is only of value
if it gets diligently maintained. Putting the data in a different tool
changes nothing about that. (A tool might reduce the effort involved,
*if* it's a good match to your workflow. But it won't reduce the effort
to zero.)
Consulting the wiki page's edit history shows that Bruce has been
doing some maintenance work, but almost nobody else does. Evidently
that's not enough.
I'd be +1 for the "aggressive culling" suggested upthread, but
I'm not particularly volunteering to do it, either.
regards, tom lane
On Fri, Oct 9, 2020 at 12:50:36PM -0400, Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
That said, having this in a different format would in no way fix the fact
that the information is mislabeled and obsolete. It would likely be
equally mislabeled and obsolete in an issue tracker. Just look at almost
any of the other projects out there that *do* use an issue tracker and have
been around for 20+ years.The long and the short of it is that a list like this is only of value
if it gets diligently maintained. Putting the data in a different tool
changes nothing about that. (A tool might reduce the effort involved,
*if* it's a good match to your workflow. But it won't reduce the effort
to zero.)Consulting the wiki page's edit history shows that Bruce has been
doing some maintenance work, but almost nobody else does. Evidently
that's not enough.I'd be +1 for the "aggressive culling" suggested upthread, but
I'm not particularly volunteering to do it, either.
+1
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Fri, Oct 9, 2020 at 12:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Consulting the wiki page's edit history shows that Bruce has been
doing some maintenance work, but almost nobody else does. Evidently
that's not enough.I'd be +1 for the "aggressive culling" suggested upthread, but
I'm not particularly volunteering to do it, either.
I'll start a separate thread for this. Working on one or a small number of
sections at a time should be manageable.
--
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company