Fixing memory leak in pg_upgrade
According to Coverity, there's a memory leak bug in transfer_all_new_dbs().
mappings = gen_db_file_maps(old_db, new_db, &n_maps, old_pgdata,
new_pgdata);
if (n_maps)
{
print_maps(mappings, n_maps, new_db->db_name);
#ifdef PAGE_CONVERSION
pageConverter = setupPageConverter();
#endif
transfer_single_new_db(pageConverter, mappings, n_maps,
old_tablespace);
pg_free(mappings);
}
-----> leaks "mappings"
}
return;
}
This is because gen_db_file_maps() allocates memory even if n_maps == 0.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 9, 2015 at 9:23 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:
This is because gen_db_file_maps() allocates memory even if n_maps == 0.
Purely cosmetic: the initialization n_maps = 0 before the call of
gen_db_file_maps is unnecessary ;)
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tatsuo Ishii <ishii@postgresql.org> writes:
According to Coverity, there's a memory leak bug in transfer_all_new_dbs().
It's pretty difficult to get excited about that; how many table-free
databases is pg_upgrade likely to see in one run? But surely we could
just move the pg_free call to after the if-block.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 9, 2015 at 11:34:24AM -0500, Tom Lane wrote:
Tatsuo Ishii <ishii@postgresql.org> writes:
According to Coverity, there's a memory leak bug in transfer_all_new_dbs().
It's pretty difficult to get excited about that; how many table-free
databases is pg_upgrade likely to see in one run? But surely we could
just move the pg_free call to after the if-block.
I have fixed this with the attached, applied patch. I thought malloc(0)
would return null, but our src/common pg_malloc() has:
/* Avoid unportable behavior of malloc(0) */
if (size == 0)
size = 1;
so some memory is allocated, and has to be freed. I looked at avoiding
the call to gen_db_file_maps() for old_db->rel_arr.nrels == 0, but there
are checks in there comparing the old/new relation counts, so it can't
be skipped. I also removed the unnecessary memory initialization.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
difftext/plain; charset=us-asciiDownload
commit ac7009abd228362042edd10e6b12556ddef35171
Author: Bruce Momjian <bruce@momjian.us>
Date: Fri Jan 9 12:12:30 2015 -0500
pg_upgrade: fix one-byte per empty db memory leak
Report by Tatsuo Ishii, Coverity
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 70753f2..423802b
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*************** transfer_all_new_dbs(DbInfoArr *old_db_a
*** 110,119 ****
pg_fatal("old database \"%s\" not found in the new cluster\n",
old_db->db_name);
- n_maps = 0;
mappings = gen_db_file_maps(old_db, new_db, &n_maps, old_pgdata,
new_pgdata);
-
if (n_maps)
{
print_maps(mappings, n_maps, new_db->db_name);
--- 110,117 ----
*************** transfer_all_new_dbs(DbInfoArr *old_db_a
*** 123,131 ****
#endif
transfer_single_new_db(pageConverter, mappings, n_maps,
old_tablespace);
-
- pg_free(mappings);
}
}
return;
--- 121,129 ----
#endif
transfer_single_new_db(pageConverter, mappings, n_maps,
old_tablespace);
}
+ /* We allocate something even for n_maps == 0 */
+ pg_free(mappings);
}
return;
On Fri, Jan 9, 2015 at 9:23 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:
This is because gen_db_file_maps() allocates memory even if n_maps == 0.
Purely cosmetic: the initialization n_maps = 0 before the call of
gen_db_file_maps is unnecessary ;)
Of course. n_maps is written by calling gen_db_file_maps() anyway. I
was talking about the case after calling gen_db_file_maps().
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jan 10, 2015 at 2:27 AM, Bruce Momjian <bruce@momjian.us> wrote:
so some memory is allocated, and has to be freed. I looked at avoiding
the call to gen_db_file_maps() for old_db->rel_arr.nrels == 0, but there
are checks in there comparing the old/new relation counts, so it can't
be skipped. I also removed the unnecessary memory initialization.
Patch is fine IMO for its purpose.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers