ParseTzFile doesn't FreeFile on error
While working on some patch, I saw the following error message when a
transaction ended successfully after a failed call to
parse_and_validate_value().
The cause is ParseTzFile() returns leaving an open file descriptor
unfreed in some error cases.
This happens only in a special case when the errors are ignored, but
in principle the file descriptor should be released before exiting the
function.
I'm not sure it's worth fixing but the attached fixes that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Fix-ParseTzFile-to-call-FreeFile-on-error.patchtext/x-patch; charset=us-asciiDownload
From a81f2fd6f0293f3e813575d76a59beda93bc030a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 30 May 2022 15:49:16 +0900
Subject: [PATCH 1/2] Fix ParseTzFile to call FreeFile() on error
ParseTzFile() forgot to FreeFile() zone file on error. Thus when this
is called within a successfully-ended transaction, it leaves an open
file descriptor, which leads to the error "n temporary files and
directories not closed at end-of-transaction".
---
src/backend/utils/misc/tzparser.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/misc/tzparser.c b/src/backend/utils/misc/tzparser.c
index a69cb2d268..bfb259b3c3 100644
--- a/src/backend/utils/misc/tzparser.c
+++ b/src/backend/utils/misc/tzparser.c
@@ -364,7 +364,7 @@ ParseTzFile(const char *filename, int depth,
{
GUC_check_errmsg("could not read time zone file \"%s\": %m",
filename);
- return -1;
+ goto error_exit;
}
/* else we're at EOF after all */
break;
@@ -374,7 +374,7 @@ ParseTzFile(const char *filename, int depth,
/* the line is too long for tzbuf */
GUC_check_errmsg("line is too long in time zone file \"%s\", line %d",
filename, lineno);
- return -1;
+ goto error_exit;
}
/* skip over whitespace */
@@ -397,12 +397,12 @@ ParseTzFile(const char *filename, int depth,
{
GUC_check_errmsg("@INCLUDE without file name in time zone file \"%s\", line %d",
filename, lineno);
- return -1;
+ goto error_exit;
}
n = ParseTzFile(includeFile, depth + 1,
base, arraysize, n);
if (n < 0)
- return -1;
+ goto error_exit;
continue;
}
@@ -413,14 +413,19 @@ ParseTzFile(const char *filename, int depth,
}
if (!splitTzLine(filename, lineno, line, &tzentry))
- return -1;
+ goto error_exit;
if (!validateTzEntry(&tzentry))
- return -1;
+ goto error_exit;
n = addToArray(base, arraysize, n, &tzentry, override);
if (n < 0)
- return -1;
+ goto error_exit;
}
+ goto success;
+error_exit:
+ n = -1;
+
+success:
FreeFile(tzFile);
return n;
--
2.31.1
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
The cause is ParseTzFile() returns leaving an open file descriptor
unfreed in some error cases.
This happens only in a special case when the errors are ignored, but
in principle the file descriptor should be released before exiting the
function.
I'm not sure it's worth fixing but the attached fixes that.
I agree this is worth fixing, but adding all these gotos seems a bit
inelegant. What do you think of the attached version?
BTW, my first thought about it was "what if one of the callees throws
elog(ERROR), eg palloc out-of-memory"? But I think that's all right
since then we'll reach transaction abort cleanup, which won't whine
about open files. The problem is limited to the case where no error
gets thrown.
regards, tom lane
Attachments:
0002-Fix-ParseTzFile-to-call-FreeFile-on-error.patchtext/x-diff; charset=us-ascii; name=0002-Fix-ParseTzFile-to-call-FreeFile-on-error.patchDownload
diff --git a/src/backend/utils/misc/tzparser.c b/src/backend/utils/misc/tzparser.c
index a69cb2d268..8f2c95f055 100644
--- a/src/backend/utils/misc/tzparser.c
+++ b/src/backend/utils/misc/tzparser.c
@@ -364,7 +364,8 @@ ParseTzFile(const char *filename, int depth,
{
GUC_check_errmsg("could not read time zone file \"%s\": %m",
filename);
- return -1;
+ n = -1;
+ break;
}
/* else we're at EOF after all */
break;
@@ -374,7 +375,8 @@ ParseTzFile(const char *filename, int depth,
/* the line is too long for tzbuf */
GUC_check_errmsg("line is too long in time zone file \"%s\", line %d",
filename, lineno);
- return -1;
+ n = -1;
+ break;
}
/* skip over whitespace */
@@ -397,12 +399,13 @@ ParseTzFile(const char *filename, int depth,
{
GUC_check_errmsg("@INCLUDE without file name in time zone file \"%s\", line %d",
filename, lineno);
- return -1;
+ n = -1;
+ break;
}
n = ParseTzFile(includeFile, depth + 1,
base, arraysize, n);
if (n < 0)
- return -1;
+ break;
continue;
}
@@ -413,12 +416,18 @@ ParseTzFile(const char *filename, int depth,
}
if (!splitTzLine(filename, lineno, line, &tzentry))
- return -1;
+ {
+ n = -1;
+ break;
+ }
if (!validateTzEntry(&tzentry))
- return -1;
+ {
+ n = -1;
+ break;
+ }
n = addToArray(base, arraysize, n, &tzentry, override);
if (n < 0)
- return -1;
+ break;
}
FreeFile(tzFile);
At Mon, 30 May 2022 13:11:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
The cause is ParseTzFile() returns leaving an open file descriptor
unfreed in some error cases.
This happens only in a special case when the errors are ignored, but
in principle the file descriptor should be released before exiting the
function.
I'm not sure it's worth fixing but the attached fixes that.I agree this is worth fixing, but adding all these gotos seems a bit
inelegant. What do you think of the attached version?
It is what came up to me first. It is natural. So I'm fine with
it. The point of the "goto"s was that repeated "n = -1;break;" looked
somewhat noisy to me in the loop.
BTW, my first thought about it was "what if one of the callees throws
elog(ERROR), eg palloc out-of-memory"? But I think that's all right
since then we'll reach transaction abort cleanup, which won't whine
about open files. The problem is limited to the case where no error
gets thrown.
Right. This "issue" is not a problem unless the caller continues
without throwing an exception after the function errors out, which is
not done by the current code.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
At Mon, 30 May 2022 13:11:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
BTW, my first thought about it was "what if one of the callees throws
elog(ERROR), eg palloc out-of-memory"? But I think that's all right
since then we'll reach transaction abort cleanup, which won't whine
about open files. The problem is limited to the case where no error
gets thrown.
Right. This "issue" is not a problem unless the caller continues
without throwing an exception after the function errors out, which is
not done by the current code.
Actually the problem *is* reachable, if you intentionally break the
already-active timezone abbreviation file: newly started sessions
produce file-leak warnings after failing to apply the setting.
I concede that's not a likely scenario, but that's why I think it's
worth fixing.
regards, tom lane
At Tue, 31 May 2022 14:21:28 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Actually the problem *is* reachable, if you intentionally break the
already-active timezone abbreviation file: newly started sessions
produce file-leak warnings after failing to apply the setting.
I concede that's not a likely scenario, but that's why I think it's
worth fixing.
Ah, I see. Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center