From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Joe Conway <mail@joeconway.com>
Subject: get_controlfile() can leak fds in the backend
Date: Wed, 27 Feb 2019 16:47:28 +0900
Message-ID: <20190227074728.GA15710@paquier.xyz> (raw)
Hi all,
(CC-ing Joe as of dc7d70e)
I was just looking at the offline checksum patch, and noticed some
sloppy coding in controldata_utils.c. The control file gets opened in
get_controlfile(), and if it generates an error then the file
descriptor remains open. As basic code rule in the backend we should
only use BasicOpenFile() when opening files, so I think that the issue
should be fixed as attached, even if this requires including fd.h for
the backend compilation which is kind of ugly.
Another possibility would be to just close the file descriptor before
any error, saving appropriately errno before issuing any %m portion,
but that still does not respect the backend policy regarding open().
We also do not have a wait event for the read() call, maybe we should
have one, but knowing that this gets called only for the SQL-level
functions accessing the control file, I don't really think that's
worth it.
Thoughts?
--
Michael
Attachments:
[text/x-diff] controlfile-leak.patch (1.0K, ../20190227074728.GA15710@paquier.xyz/2-controlfile-leak.patch)
download | inline diff:diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.cindex abfe7065f5..0b011741e3 100644--- a/src/common/controldata_utils.c+++ b/src/common/controldata_utils.c@@ -27,6 +27,9 @@
#include "catalog/pg_control.h"
#include "common/controldata_utils.h"
#include "port/pg_crc32c.h"
+#ifndef FRONTEND+#include "storage/fd.h"+#endif
/*
* get_controlfile(char *DataDir, const char *progname, bool *crc_ok_p)
@@ -45,13 +48,20 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
char ControlFilePath[MAXPGPATH];
pg_crc32c crc;
int r;
+ int flags = O_RDONLY | PG_BINARY;
AssertArg(crc_ok_p);
ControlFile = palloc(sizeof(ControlFileData));
snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
- if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)+#ifndef FRONTEND+ fd = BasicOpenFile(ControlFilePath, flags);+#else+ fd = open(ControlFilePath, flags, 0);+#endif++ if (fd < 0)
#ifndef FRONTEND
ereport(ERROR,
(errcode_for_file_access(),
[application/pgp-signature] signature.asc (832B, ../20190227074728.GA15710@paquier.xyz/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, pgsql-hackers@lists.postgresql.org, mail@joeconway.com
Subject: Re: get_controlfile() can leak fds in the backend
In-Reply-To: <20190227074728.GA15710@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox