Thread: [HACKERS] Memory leak
Hi all,
While reading the code, I find some code that make memory leak:
- src/port/quotes.c
At line 38, at function "escape_single_quotes_ascii",
here used "malloc" to get some memory and return the
pointer returned by the "malloc".
So, any caller used this function shoule free this memory.
- /src/bin/initdb/initdb.c
At line 327, at function "escape_quotes",
which use function "escape_single_quotes_ascii"
from above file.
But at this file(/src/bin/initdb/initdb.c), there are many place
used function "escape_quotes" but have not free the pointer returned
by the function, thus cause memory leak.
I have fixed this bug and made a patch here:
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 7303bbe..4e5429e 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1010,6 +1010,7 @@ setup_config(void)
char path[MAXPGPATH];
const char *default_timezone;
char *autoconflines[3];
+ char *escaped;
fputs(_("creating configuration files ... "), stdout);
fflush(stdout);
@@ -1043,20 +1044,28 @@ setup_config(void)
conflines = replace_token(conflines, "#port = 5432", repltok);
#endif
+ escaped = escape_quotes(lc_messages);
snprintf(repltok, sizeof(repltok), "lc_messages = '%s'",
- escape_quotes(lc_messages));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_messages = 'C'", repltok);
+ escaped = escape_quotes(lc_monetary);
snprintf(repltok, sizeof(repltok), "lc_monetary = '%s'",
- escape_quotes(lc_monetary));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_monetary = 'C'", repltok);
+ escaped = escape_quotes(lc_numeric);
snprintf(repltok, sizeof(repltok), "lc_numeric = '%s'",
- escape_quotes(lc_numeric));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_numeric = 'C'", repltok);
+ escaped = escape_quotes(lc_time);
snprintf(repltok, sizeof(repltok), "lc_time = '%s'",
- escape_quotes(lc_time));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_time = 'C'", repltok);
switch (locale_date_order(lc_time))
@@ -1074,9 +1083,11 @@ setup_config(void)
}
conflines = replace_token(conflines, "#datestyle = 'iso, mdy'", repltok);
+ escaped = escape_quotes(default_text_search_config);
snprintf(repltok, sizeof(repltok),
"default_text_search_config = 'pg_catalog.%s'",
- escape_quotes(default_text_search_config));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines,
"#default_text_search_config = 'pg_catalog.simple'",
repltok);
@@ -1084,11 +1095,13 @@ setup_config(void)
default_timezone = select_default_timezone(share_path);
if (default_timezone)
{
+ escaped = escape_quotes(default_timezone);
snprintf(repltok, sizeof(repltok), "timezone = '%s'",
- escape_quotes(default_timezone));
+ escaped);
conflines = replace_token(conflines, "#timezone = 'GMT'", repltok);
snprintf(repltok, sizeof(repltok), "log_timezone = '%s'",
- escape_quotes(default_timezone));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#log_timezone = 'GMT'", repltok);
}
@@ -1289,6 +1302,7 @@ bootstrap_template1(void)
char **bki_lines;
char headerline[MAXPGPATH];
char buf[64];
+ char *escaped;
printf(_("running bootstrap script ... "));
fflush(stdout);
@@ -1330,13 +1344,19 @@ bootstrap_template1(void)
bki_lines = replace_token(bki_lines, "FLOAT8PASSBYVAL",
FLOAT8PASSBYVAL ? "true" : "false");
- bki_lines = replace_token(bki_lines, "POSTGRES", escape_quotes(username));
+ escaped = escape_quotes(username);
+ bki_lines = replace_token(bki_lines, "POSTGRES", escaped);
+ free(escaped);
bki_lines = replace_token(bki_lines, "ENCODING", encodingid);
- bki_lines = replace_token(bki_lines, "LC_COLLATE", escape_quotes(lc_collate));
+ escaped = escape_quotes(lc_collate);
+ bki_lines = replace_token(bki_lines, "LC_COLLATE", escaped);
+ free(escaped);
- bki_lines = replace_token(bki_lines, "LC_CTYPE", escape_quotes(lc_ctype));
+ escaped = escape_quotes(lc_ctype);
+ bki_lines = replace_token(bki_lines, "LC_CTYPE", escaped);
+ free(escaped);
/*
* Pass correct LC_xxx environment to bootstrap.
@@ -1391,13 +1411,16 @@ setup_auth(FILE *cmdfd)
"REVOKE ALL on pg_authid FROM public;\n\n",
NULL
};
+ char *escaped;
for (line = pg_authid_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
+ escaped = escape_quotes(superuser_password);
if (superuser_password)
PG_CMD_PRINTF2("ALTER USER \"%s\" WITH PASSWORD E'%s';\n\n",
- username, escape_quotes(superuser_password));
+ username, escaped);
+ free(escaped);
}
/*
@@ -1582,14 +1605,18 @@ setup_sysviews(FILE *cmdfd)
static void
setup_description(FILE *cmdfd)
{
+ char *escaped;
+
PG_CMD_PUTS("CREATE TEMP TABLE tmp_pg_description ( "
" objoid oid, "
" classname name, "
" objsubid int4, "
" description text) WITHOUT OIDS;\n\n");
+ escaped = escape_quotes(desc_file);
PG_CMD_PRINTF1("COPY tmp_pg_description FROM E'%s';\n\n",
- escape_quotes(desc_file));
+ escaped);
+ free(escaped);
PG_CMD_PUTS("INSERT INTO pg_description "
" SELECT t.objoid, c.oid, t.objsubid, t.description "
@@ -1601,8 +1628,10 @@ setup_description(FILE *cmdfd)
" classname name, "
" description text) WITHOUT OIDS;\n\n");
+ escaped = escape_quotes(shdesc_file);
PG_CMD_PRINTF1("COPY tmp_pg_shdescription FROM E'%s';\n\n",
- escape_quotes(shdesc_file));
+ escaped);
+ free(escaped);
PG_CMD_PUTS("INSERT INTO pg_shdescription "
" SELECT t.objoid, c.oid, t.description "
@@ -1846,9 +1875,13 @@ setup_privileges(FILE *cmdfd)
" srvacl IS NOT NULL;",
NULL
};
+ char *escaped;
+ escaped = escape_quotes(username);
priv_lines = replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME",
- escape_quotes(username));
+ escaped);
+ free(escaped);
+
for (line = priv_lines; *line != NULL; line++)
PG_CMD_PUTS(*line);
}
@@ -1889,6 +1922,7 @@ setup_schema(FILE *cmdfd)
{
char **line;
char **lines;
+ char *escaped;
lines = readfile(info_schema_file);
@@ -1905,11 +1939,13 @@ setup_schema(FILE *cmdfd)
" WHERE implementation_info_name = 'DBMS VERSION';\n\n",
infoversion);
+ escaped = escape_quotes(features_file);
PG_CMD_PRINTF1("COPY information_schema.sql_features "
" (feature_id, feature_name, sub_feature_id, "
" sub_feature_name, is_supported, comments) "
" FROM E'%s';\n\n",
- escape_quotes(features_file));
+ escaped);
+ free(escaped);
}
/*
Hope this patch can fix this problem.
--
Yang Fan
While reading the code, I find some code that make memory leak:
- src/port/quotes.c
At line 38, at function "escape_single_quotes_ascii",
here used "malloc" to get some memory and return the
pointer returned by the "malloc".
So, any caller used this function shoule free this memory.
- /src/bin/initdb/initdb.c
At line 327, at function "escape_quotes",
which use function "escape_single_quotes_ascii"
from above file.
But at this file(/src/bin/initdb/initdb.c), there are many place
used function "escape_quotes" but have not free the pointer returned
by the function, thus cause memory leak.
I have fixed this bug and made a patch here:
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 7303bbe..4e5429e 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1010,6 +1010,7 @@ setup_config(void)
char path[MAXPGPATH];
const char *default_timezone;
char *autoconflines[3];
+ char *escaped;
fputs(_("creating configuration files ... "), stdout);
fflush(stdout);
@@ -1043,20 +1044,28 @@ setup_config(void)
conflines = replace_token(conflines, "#port = 5432", repltok);
#endif
+ escaped = escape_quotes(lc_messages);
snprintf(repltok, sizeof(repltok), "lc_messages = '%s'",
- escape_quotes(lc_messages));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_messages = 'C'", repltok);
+ escaped = escape_quotes(lc_monetary);
snprintf(repltok, sizeof(repltok), "lc_monetary = '%s'",
- escape_quotes(lc_monetary));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_monetary = 'C'", repltok);
+ escaped = escape_quotes(lc_numeric);
snprintf(repltok, sizeof(repltok), "lc_numeric = '%s'",
- escape_quotes(lc_numeric));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_numeric = 'C'", repltok);
+ escaped = escape_quotes(lc_time);
snprintf(repltok, sizeof(repltok), "lc_time = '%s'",
- escape_quotes(lc_time));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#lc_time = 'C'", repltok);
switch (locale_date_order(lc_time))
@@ -1074,9 +1083,11 @@ setup_config(void)
}
conflines = replace_token(conflines, "#datestyle = 'iso, mdy'", repltok);
+ escaped = escape_quotes(default_text_search_config);
snprintf(repltok, sizeof(repltok),
"default_text_search_config = 'pg_catalog.%s'",
- escape_quotes(default_text_search_config));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines,
"#default_text_search_config = 'pg_catalog.simple'",
repltok);
@@ -1084,11 +1095,13 @@ setup_config(void)
default_timezone = select_default_timezone(share_path);
if (default_timezone)
{
+ escaped = escape_quotes(default_timezone);
snprintf(repltok, sizeof(repltok), "timezone = '%s'",
- escape_quotes(default_timezone));
+ escaped);
conflines = replace_token(conflines, "#timezone = 'GMT'", repltok);
snprintf(repltok, sizeof(repltok), "log_timezone = '%s'",
- escape_quotes(default_timezone));
+ escaped);
+ free(escaped);
conflines = replace_token(conflines, "#log_timezone = 'GMT'", repltok);
}
@@ -1289,6 +1302,7 @@ bootstrap_template1(void)
char **bki_lines;
char headerline[MAXPGPATH];
char buf[64];
+ char *escaped;
printf(_("running bootstrap script ... "));
fflush(stdout);
@@ -1330,13 +1344,19 @@ bootstrap_template1(void)
bki_lines = replace_token(bki_lines, "FLOAT8PASSBYVAL",
FLOAT8PASSBYVAL ? "true" : "false");
- bki_lines = replace_token(bki_lines, "POSTGRES", escape_quotes(username));
+ escaped = escape_quotes(username);
+ bki_lines = replace_token(bki_lines, "POSTGRES", escaped);
+ free(escaped);
bki_lines = replace_token(bki_lines, "ENCODING", encodingid);
- bki_lines = replace_token(bki_lines, "LC_COLLATE", escape_quotes(lc_collate));
+ escaped = escape_quotes(lc_collate);
+ bki_lines = replace_token(bki_lines, "LC_COLLATE", escaped);
+ free(escaped);
- bki_lines = replace_token(bki_lines, "LC_CTYPE", escape_quotes(lc_ctype));
+ escaped = escape_quotes(lc_ctype);
+ bki_lines = replace_token(bki_lines, "LC_CTYPE", escaped);
+ free(escaped);
/*
* Pass correct LC_xxx environment to bootstrap.
@@ -1391,13 +1411,16 @@ setup_auth(FILE *cmdfd)
"REVOKE ALL on pg_authid FROM public;\n\n",
NULL
};
+ char *escaped;
for (line = pg_authid_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
+ escaped = escape_quotes(superuser_password);
if (superuser_password)
PG_CMD_PRINTF2("ALTER USER \"%s\" WITH PASSWORD E'%s';\n\n",
- username, escape_quotes(superuser_password));
+ username, escaped);
+ free(escaped);
}
/*
@@ -1582,14 +1605,18 @@ setup_sysviews(FILE *cmdfd)
static void
setup_description(FILE *cmdfd)
{
+ char *escaped;
+
PG_CMD_PUTS("CREATE TEMP TABLE tmp_pg_description ( "
" objoid oid, "
" classname name, "
" objsubid int4, "
" description text) WITHOUT OIDS;\n\n");
+ escaped = escape_quotes(desc_file);
PG_CMD_PRINTF1("COPY tmp_pg_description FROM E'%s';\n\n",
- escape_quotes(desc_file));
+ escaped);
+ free(escaped);
PG_CMD_PUTS("INSERT INTO pg_description "
" SELECT t.objoid, c.oid, t.objsubid, t.description "
@@ -1601,8 +1628,10 @@ setup_description(FILE *cmdfd)
" classname name, "
" description text) WITHOUT OIDS;\n\n");
+ escaped = escape_quotes(shdesc_file);
PG_CMD_PRINTF1("COPY tmp_pg_shdescription FROM E'%s';\n\n",
- escape_quotes(shdesc_file));
+ escaped);
+ free(escaped);
PG_CMD_PUTS("INSERT INTO pg_shdescription "
" SELECT t.objoid, c.oid, t.description "
@@ -1846,9 +1875,13 @@ setup_privileges(FILE *cmdfd)
" srvacl IS NOT NULL;",
NULL
};
+ char *escaped;
+ escaped = escape_quotes(username);
priv_lines = replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME",
- escape_quotes(username));
+ escaped);
+ free(escaped);
+
for (line = priv_lines; *line != NULL; line++)
PG_CMD_PUTS(*line);
}
@@ -1889,6 +1922,7 @@ setup_schema(FILE *cmdfd)
{
char **line;
char **lines;
+ char *escaped;
lines = readfile(info_schema_file);
@@ -1905,11 +1939,13 @@ setup_schema(FILE *cmdfd)
" WHERE implementation_info_name = 'DBMS VERSION';\n\n",
infoversion);
+ escaped = escape_quotes(features_file);
PG_CMD_PRINTF1("COPY information_schema.sql_features "
" (feature_id, feature_name, sub_feature_id, "
" sub_feature_name, is_supported, comments) "
" FROM E'%s';\n\n",
- escape_quotes(features_file));
+ escaped);
+ free(escaped);
}
/*
Hope this patch can fix this problem.
--
Yang Fan
fan yang <yangfangoto@gmail.com> writes: > - src/port/quotes.c > At line 38, at function "escape_single_quotes_ascii", > here used "malloc" to get some memory and return the > pointer returned by the "malloc". > So, any caller used this function shoule free this memory. > - /src/bin/initdb/initdb.c > At line 327, at function "escape_quotes", > which use function "escape_single_quotes_ascii" > from above file. > But at this file(/src/bin/initdb/initdb.c), there are many place > used function "escape_quotes" but have not free the pointer returned > by the function, thus cause memory leak. By and large, we intentionally don't worry about memory leaks in initdb (or any other program with a limited runtime). It's not worth the maintenance effort to save a few bytes, at least not where it requires code contortions like these. Doing a quick check with valgrind says that a run of initdb, in HEAD, leaks about 560KB over its lifespan. That's well below the threshold of pain on any machine capable of running modern Postgres reasonably. For fun, I tried to see whether your patch moved that number appreciably, but patch(1) couldn't make any sense of it at all. I think that probably your mail program munged the whitespace in the patch. Many of us have found that the most reliable way to forward patches through email is to add them as attachments rather than pasting them into the text in-line. Poking at it a bit harder with valgrind, it seems that the vast majority of what it reports as leaks is caused by replace_token(). If we wanted to reduce memory wastage during initdb, that would be the place to work on. But I'm dubious that it's worth any effort. regards, tom lane
You are right. When I add those code, it really give me a strong bad smell. It not worth these effort.
Thanks for your reply and suggestion.
--
Sincerely
Fan Yang