Plug minor memleak in pg_dump
Hi,
I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer which
should then be freed. While reading the Table of Contents, it was called as an argument
within a function call, leading to a memleak.
Please accept the attached as a proposed fix.
Cheers,
//Georgios
Attachments:
v1-0001-Plug-minor-leak-while-reading-Table-of-Contents.patchtext/x-patch; name=v1-0001-Plug-minor-leak-while-reading-Table-of-Contents.patchDownload
From 9473d1106c2816550094f5dea939fc3c6a469f4d Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokolatos@pm.me>
Date: Tue, 1 Feb 2022 13:08:48 +0000
Subject: [PATCH v1] Plug minor leak while reading Table of Contents
ReadStr() returns a malloc'ed pointer. Using it directly in a function
call results in a memleak. Rewrite to use a temporary buffer which is
then freed.
---
src/bin/pg_dump/pg_backup_archiver.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 49bf090..b2d7d21 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2574,8 +2574,15 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
+ if (AH->version < K_VERS_1_9)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ else
+ {
+ tmp = ReadStr(AH);
+ if (strcmp(tmp, "true") == 0)
+ pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ pg_free(tmp);
+ }
/* Read TOC entry dependencies */
if (AH->version >= K_VERS_1_5)
--
2.34.1
On Tue, Feb 1, 2022 at 7:06 PM <gkokolatos@pm.me> wrote:
Hi,
I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer which
should then be freed. While reading the Table of Contents, it was called as an argument
within a function call, leading to a memleak.Please accept the attached as a proposed fix.
+1. IMO, having "restoring tables WITH OIDS is not supported anymore"
twice doesn't look good, how about as shown in [1]diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 49bf0907cd..777ff6fcfe 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -2494,6 +2494,7 @@ ReadToc(ArchiveHandle *AH) int depIdx; int depSize; TocEntry *te; + bool is_supported = true;?
[1]
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 49bf0907cd..777ff6fcfe 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2494,6 +2494,7 @@ ReadToc(ArchiveHandle *AH)
int depIdx;
int depSize;
TocEntry *te;
+ bool is_supported = true; AH->tocCount = ReadInt(AH);
AH->maxDumpId = 0;
@@ -2574,7 +2575,20 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH),
"true") == 0)
+
+ if (AH->version < K_VERS_1_9)
+ is_supported = false;
+ else
+ {
+ tmp = ReadStr(AH);
+
+ if (strcmp(tmp, "true") == 0)
+ is_supported = false;
+
+ pg_free(tmp);
+ }
+
+ if (!is_supported)
pg_log_warning("restoring tables WITH OIDS is
not supported anymore");/* Read TOC entry dependencies */
Regards,
Bharath Rupireddy.
At Tue, 1 Feb 2022 19:48:01 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
On Tue, Feb 1, 2022 at 7:06 PM <gkokolatos@pm.me> wrote:
Hi,
I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer which
should then be freed. While reading the Table of Contents, it was called as an argument
within a function call, leading to a memleak.Please accept the attached as a proposed fix.
It is freed in other temporary use of the result of ReadStr(). So
freeing it sounds sensible at a glance.
+1. IMO, having "restoring tables WITH OIDS is not supported anymore"
twice doesn't look good, how about as shown in [1]?
Maybe [2] is smaller :)
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2494,6 +2494,7 @@ ReadToc(ArchiveHandle *AH)
int depIdx;
int depSize;
TocEntry *te;
+ char *tmpstr = NULL; AH->tocCount = ReadInt(AH);
AH->maxDumpId = 0;
@@ -2574,8 +2575,14 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
+ if (AH->version < K_VERS_1_9 ||
+ strcmp((tmpstr = ReadStr(AH)), "true") == 0)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ if (tmpstr)
+ {
+ pg_free(tmpstr);
+ tmpstr = NULL;
+ }/* Read TOC entry dependencies */
Thus.. I came to doubt of its worthiness to the complexity. The
amount of the leak is (perhaps) negligible.
So, I would just write a comment there.
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2574,6 +2574,8 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH); te->owner = ReadStr(AH);
+
+ /* deliberately leak the result of ReadStr for simplicity */
if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2 Feb 2022, at 09:29, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Tue, 1 Feb 2022 19:48:01 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
On Tue, Feb 1, 2022 at 7:06 PM <gkokolatos@pm.me> wrote:
Hi,
I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer which
should then be freed. While reading the Table of Contents, it was called as an argument
within a function call, leading to a memleak.Please accept the attached as a proposed fix.
It is freed in other temporary use of the result of ReadStr(). So
freeing it sounds sensible at a glance.+1. IMO, having "restoring tables WITH OIDS is not supported anymore"
twice doesn't look good, how about as shown in [1]?Maybe [2] is smaller :)
It is smaller, but I think Bharath's version wins in terms of readability.
Thus.. I came to doubt of its worthiness to the complexity. The
amount of the leak is (perhaps) negligible.So, I would just write a comment there.
The leak itself is clearly not something to worry about wrt memory pressure.
We do read into tmp and free it in other places in the same function though (as
you note above), so for code consistency alone this is worth doing IMO (and it
reduces the risk of static analyzers flagging this).
Unless objected to I will go ahead with getting this committed.
--
Daniel Gustafsson https://vmware.com/
On Wed, Feb 02, 2022 at 10:06:13AM +0100, Daniel Gustafsson wrote:
The leak itself is clearly not something to worry about wrt memory pressure.
We do read into tmp and free it in other places in the same function though (as
you note above), so for code consistency alone this is worth doing IMO (and it
reduces the risk of static analyzers flagging this).Unless objected to I will go ahead with getting this committed.
Looks like you forgot to apply that?
--
Michael
On Wed, Feb 9, 2022 at 8:26 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 02, 2022 at 10:06:13AM +0100, Daniel Gustafsson wrote:
The leak itself is clearly not something to worry about wrt memory pressure.
We do read into tmp and free it in other places in the same function though (as
you note above), so for code consistency alone this is worth doing IMO (and it
reduces the risk of static analyzers flagging this).Unless objected to I will go ahead with getting this committed.
Looks like you forgot to apply that?
Attaching the patch that I suggested above, also the original patch
proposed by Georgios is at [1]/messages/by-id/oZwKiUxFsVaetG2xOJp7Hwao8F1AKIdfFDQLNJrnwoaxmjyB-45r_aYmhgXHKLcMI3GT24m9L6HafSi2ns7WFxXe0mw2_tIJpD-Z3vb_eyI=@pm.me, leaving the decision to the committer
to pick up the best one.
Regards,
Bharath Rupireddy.
Attachments:
v1-0001-Fix-a-memory-leak-while-reading-Table-of-Contents.patchapplication/x-patch; name=v1-0001-Fix-a-memory-leak-while-reading-Table-of-Contents.patchDownload
From 335db3331a99a6cfc35608cdbec204d843b8ac55 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 9 Feb 2022 03:25:50 +0000
Subject: [PATCH v1] Fix a memory leak while reading Table of Contents
ReadStr() returns a malloc'ed pointer. Using it directly in a function
call results in a memleak. Rewrite to use a temporary buffer which is
then freed.
---
src/bin/pg_dump/pg_backup_archiver.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 49bf0907cd..e62be78982 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2500,6 +2500,8 @@ ReadToc(ArchiveHandle *AH)
for (i = 0; i < AH->tocCount; i++)
{
+ bool is_supported = true;
+
te = (TocEntry *) pg_malloc0(sizeof(TocEntry));
te->dumpId = ReadInt(AH);
@@ -2574,7 +2576,20 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
+
+ if (AH->version < K_VERS_1_9)
+ is_supported = false;
+ else
+ {
+ tmp = ReadStr(AH);
+
+ if (strcmp(tmp, "true") == 0)
+ is_supported = false;
+
+ pg_free(tmp);
+ }
+
+ if (!is_supported)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");
/* Read TOC entry dependencies */
--
2.25.1
On 9 Feb 2022, at 03:56, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 02, 2022 at 10:06:13AM +0100, Daniel Gustafsson wrote:
The leak itself is clearly not something to worry about wrt memory pressure.
We do read into tmp and free it in other places in the same function though (as
you note above), so for code consistency alone this is worth doing IMO (and it
reduces the risk of static analyzers flagging this).Unless objected to I will go ahead with getting this committed.
Looks like you forgot to apply that?
No, but I was distracted by other things leaving this on the TODO list. It's
been pushed now.
--
Daniel Gustafsson https://vmware.com/
No, but I was distracted by other things leaving this on the TODO list.
It's
been pushed now.
Hi,
IMO I think that still have troubles here.
ReadStr can return NULL, so the fix can crash.
regards,
Ranier Vilela
Attachments:
v1_fix_possible_null_dereference_pg_backup_archiver.patchapplication/octet-stream; name=v1_fix_possible_null_dereference_pg_backup_archiver.patchDownload
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index d41a99d6ea..2b781a39b6 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2040,21 +2040,22 @@ WriteStr(ArchiveHandle *AH, const char *c)
char *
ReadStr(ArchiveHandle *AH)
{
- char *buf;
int l;
l = ReadInt(AH);
- if (l < 0)
- buf = NULL;
- else
+ if (l > 0)
{
+ char *buf;
+
buf = (char *) pg_malloc(l + 1);
AH->ReadBufPtr(AH, (void *) buf, l);
buf[l] = '\0';
+
+ return buf;
}
- return buf;
+ return NULL;
}
static int
@@ -2494,7 +2495,6 @@ ReadToc(ArchiveHandle *AH)
int depIdx;
int depSize;
TocEntry *te;
- bool is_supported;
AH->tocCount = ReadInt(AH);
AH->maxDumpId = 0;
@@ -2517,14 +2517,24 @@ ReadToc(ArchiveHandle *AH)
if (AH->version >= K_VERS_1_8)
{
tmp = ReadStr(AH);
- sscanf(tmp, "%u", &te->catalogId.tableoid);
- free(tmp);
+ if (tmp)
+ {
+ sscanf(tmp, "%u", &te->catalogId.tableoid);
+ free(tmp);
+ }
+ else
+ te->catalogId.tableoid = InvalidOid;
}
else
te->catalogId.tableoid = InvalidOid;
tmp = ReadStr(AH);
- sscanf(tmp, "%u", &te->catalogId.oid);
- free(tmp);
+ if (tmp)
+ {
+ sscanf(tmp, "%u", &te->catalogId.oid);
+ free(tmp);
+ }
+ else
+ te->catalogId.oid = InvalidOid;
te->tag = ReadStr(AH);
te->desc = ReadStr(AH);
@@ -2575,21 +2585,22 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- is_supported = true;
if (AH->version < K_VERS_1_9)
- is_supported = false;
- else
{
- tmp = ReadStr(AH);
-
- if (strcmp(tmp, "true") == 0)
- is_supported = false;
+ bool is_supported;
+ tmp = ReadStr(AH);
+ if (tmp)
+ {
+ is_supported = (strcmp(tmp, "true") != 0);
free(tmp);
- }
+ }
+ else
+ is_supported = false;
- if (!is_supported)
- pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ if (!is_supported)
+ pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ }
/* Read TOC entry dependencies */
if (AH->version >= K_VERS_1_5)
Import Notes
Resolved by subject fallback
On Wed, Feb 09, 2022 at 02:48:35PM -0300, Ranier Vilela wrote:
IMO I think that still have troubles here.
ReadStr can return NULL, so the fix can crash.
- sscanf(tmp, "%u", &te->catalogId.tableoid);
- free(tmp);
+ if (tmp)
+ {
+ sscanf(tmp, "%u", &te->catalogId.tableoid);
+ free(tmp);
+ }
+ else
+ te->catalogId.tableoid = InvalidOid;This patch makes things worse, doesn't it? Doesn't this localized
change mean that we expose ourselves more into *ignoring* TOC entries
if we mess up with this code in the future? That sounds particularly
sensible if you have a couple of bytes corrupted in a dump.
--
Michael
Em qua., 9 de fev. de 2022 às 23:16, Michael Paquier <michael@paquier.xyz>
escreveu:
On Wed, Feb 09, 2022 at 02:48:35PM -0300, Ranier Vilela wrote:
IMO I think that still have troubles here.
ReadStr can return NULL, so the fix can crash.
- sscanf(tmp, "%u", &te->catalogId.tableoid); - free(tmp); + if (tmp) + { + sscanf(tmp, "%u", &te->catalogId.tableoid); + free(tmp); + } + else + te->catalogId.tableoid = InvalidOid;This patch makes things worse, doesn't it?
No.
Doesn't this localized
change mean that we expose ourselves more into *ignoring* TOC entries
if we mess up with this code in the future?
InvalidOid already used for "default".
If ReadStr fails and returns NULL, sscanf will crash.
Maybe in this case, better report to the user?
pg_log_warning?
regards,
Ranier Vilela
Em qui., 10 de fev. de 2022 às 08:14, Ranier Vilela <ranier.vf@gmail.com>
escreveu:
Em qua., 9 de fev. de 2022 às 23:16, Michael Paquier <michael@paquier.xyz>
escreveu:On Wed, Feb 09, 2022 at 02:48:35PM -0300, Ranier Vilela wrote:
IMO I think that still have troubles here.
ReadStr can return NULL, so the fix can crash.
- sscanf(tmp, "%u", &te->catalogId.tableoid); - free(tmp); + if (tmp) + { + sscanf(tmp, "%u", &te->catalogId.tableoid); + free(tmp); + } + else + te->catalogId.tableoid = InvalidOid;This patch makes things worse, doesn't it?
No.
Doesn't this localized
change mean that we expose ourselves more into *ignoring* TOC entries
if we mess up with this code in the future?InvalidOid already used for "default".
If ReadStr fails and returns NULL, sscanf will crash.Maybe in this case, better report to the user?
pg_log_warning?
Maybe in this case, the right thing is abort?
See v2, please.
regards,
Ranier Vilela
Attachments:
v2_fix_possible_null_dereference_pg_backup_archiver.patchapplication/octet-stream; name=v2_fix_possible_null_dereference_pg_backup_archiver.patchDownload
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index d41a99d6ea..66fff0f359 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2040,21 +2040,22 @@ WriteStr(ArchiveHandle *AH, const char *c)
char *
ReadStr(ArchiveHandle *AH)
{
- char *buf;
int l;
l = ReadInt(AH);
- if (l < 0)
- buf = NULL;
- else
+ if (l > 0)
{
+ char *buf;
+
buf = (char *) pg_malloc(l + 1);
AH->ReadBufPtr(AH, (void *) buf, l);
buf[l] = '\0';
+
+ return buf;
}
- return buf;
+ return NULL;
}
static int
@@ -2494,7 +2495,6 @@ ReadToc(ArchiveHandle *AH)
int depIdx;
int depSize;
TocEntry *te;
- bool is_supported;
AH->tocCount = ReadInt(AH);
AH->maxDumpId = 0;
@@ -2517,14 +2517,24 @@ ReadToc(ArchiveHandle *AH)
if (AH->version >= K_VERS_1_8)
{
tmp = ReadStr(AH);
- sscanf(tmp, "%u", &te->catalogId.tableoid);
- free(tmp);
+ if (tmp)
+ {
+ sscanf(tmp, "%u", &te->catalogId.tableoid);
+ free(tmp);
+ }
+ else
+ fatal("table OID out of range -- perhaps a corrupt TOC");
}
else
te->catalogId.tableoid = InvalidOid;
tmp = ReadStr(AH);
- sscanf(tmp, "%u", &te->catalogId.oid);
- free(tmp);
+ if (tmp)
+ {
+ sscanf(tmp, "%u", &te->catalogId.oid);
+ free(tmp);
+ }
+ else
+ te->catalogId.oid = InvalidOid;
te->tag = ReadStr(AH);
te->desc = ReadStr(AH);
@@ -2575,21 +2585,22 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);
te->owner = ReadStr(AH);
- is_supported = true;
if (AH->version < K_VERS_1_9)
- is_supported = false;
- else
{
- tmp = ReadStr(AH);
-
- if (strcmp(tmp, "true") == 0)
- is_supported = false;
+ bool is_supported;
+ tmp = ReadStr(AH);
+ if (tmp)
+ {
+ is_supported = (strcmp(tmp, "true") != 0);
free(tmp);
- }
+ }
+ else
+ is_supported = false;
- if (!is_supported)
- pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ if (!is_supported)
+ pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ }
/* Read TOC entry dependencies */
if (AH->version >= K_VERS_1_5)
On 10 Feb 2022, at 12:14, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em qua., 9 de fev. de 2022 às 23:16, Michael Paquier <michael@paquier.xyz <mailto:michael@paquier.xyz>> escreveu:
This patch makes things worse, doesn't it?
No.Doesn't this localized
change mean that we expose ourselves more into *ignoring* TOC entries
if we mess up with this code in the future?
InvalidOid already used for "default".
There is no default case here, setting the tableoid to InvalidOid is done when
the archive doesn't support this particular feature. If we can't read the
tableoid here, it's a corrupt TOC and we should abort.
If ReadStr fails and returns NULL, sscanf will crash.
Yes, which is better than silently propage the error.
Maybe in this case, better report to the user?
pg_log_warning?
That would demote what is today a crash to a warning on a corrupt TOC entry,
which I think is the wrong way to go. Question is, can this fail in a
non-synthetic case on output which was successfully generated by pg_dump? I'm
not saying we should ignore errors, but I have a feeling that any input fed
that triggers this will be broken enough to cause fireworks elsewhere too, and
this being a chase towards low returns apart from complicating the code.
--
Daniel Gustafsson https://vmware.com/
Em qui., 10 de fev. de 2022 às 10:57, Daniel Gustafsson <daniel@yesql.se>
escreveu:
On 10 Feb 2022, at 12:14, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em qua., 9 de fev. de 2022 às 23:16, Michael Paquier <michael@paquier.xyz <mailto:michael@paquier.xyz>> escreveu:
This patch makes things worse, doesn't it?
No.Doesn't this localized
change mean that we expose ourselves more into *ignoring* TOC entries
if we mess up with this code in the future?
InvalidOid already used for "default".There is no default case here, setting the tableoid to InvalidOid is done
when
the archive doesn't support this particular feature. If we can't read the
tableoid here, it's a corrupt TOC and we should abort.
Well, the v2 aborts.
If ReadStr fails and returns NULL, sscanf will crash.
Yes, which is better than silently propage the error.
Ok, silently propagating the error is bad, but crashing is a signal of
poor tool.
Maybe in this case, better report to the user?
pg_log_warning?That would demote what is today a crash to a warning on a corrupt TOC
entry,
which I think is the wrong way to go. Question is, can this fail in a
non-synthetic case on output which was successfully generated by pg_dump?
I'm
not saying we should ignore errors, but I have a feeling that any input fed
that triggers this will be broken enough to cause fireworks elsewhere too,
and
this being a chase towards low returns apart from complicating the code.
For me the code stays more simple and maintainable.
regards,
Ranier Vilela