closing file in adjust_data_dir
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t46929psql -h localhost -U postgresBuilt from patchset v2 (message #2), July 28, 2026 at 01:43 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t46929_2 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t46929_2 && git checkout t46929_2Patchset v2 (message #2) is on t46929_2
Hi,
I was looking at the commit:
commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Tue Nov 15 15:35:37 2022 +0100
Check return value of pclose() correctly
In src/bin/pg_ctl/pg_ctl.c :
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
pclose(fd) != 0)
If the fgets() call doesn't return NULL, the pclose() would be skipped.
Since the original pclose() call was removed, wouldn't this lead to fd
leaking ?
Please see attached patch for my proposal.
Cheers
Attachments:
pg-ctl-close-fd.patchapplication/octet-stream; name=pg-ctl-close-fd.patchDownload+6-1
On Tue, Nov 15, 2022 at 10:43 AM Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
I was looking at the commit:commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Tue Nov 15 15:35:37 2022 +0100Check return value of pclose() correctly
In src/bin/pg_ctl/pg_ctl.c :
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
pclose(fd) != 0)If the fgets() call doesn't return NULL, the pclose() would be skipped.
Since the original pclose() call was removed, wouldn't this lead to fd
leaking ?Please see attached patch for my proposal.
Cheers
There was potential leak of fd in patch v1.
Please take a look at patch v2.
Thanks
On Wed, 16 Nov 2022 at 02:43, Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
I was looking at the commit:commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Tue Nov 15 15:35:37 2022 +0100Check return value of pclose() correctly
In src/bin/pg_ctl/pg_ctl.c :
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
pclose(fd) != 0)If the fgets() call doesn't return NULL, the pclose() would be skipped.
Since the original pclose() call was removed, wouldn't this lead to fd
leaking ?Please see attached patch for my proposal.
Cheers
I think we should check whether fd is NULL or not, otherwise, segmentation
fault maybe occur.
+ if (pclose(fd) != 0)
+ {
+ write_stderr(_("%s: could not close the file following command \"%s\"\n"), progname, cmd);
+ exit(1);
+ }--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Tue, Nov 15, 2022 at 6:02 PM Japin Li <japinli@hotmail.com> wrote:
On Wed, 16 Nov 2022 at 02:43, Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
I was looking at the commit:commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Tue Nov 15 15:35:37 2022 +0100Check return value of pclose() correctly
In src/bin/pg_ctl/pg_ctl.c :
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
pclose(fd) != 0)If the fgets() call doesn't return NULL, the pclose() would be skipped.
Since the original pclose() call was removed, wouldn't this lead to fd
leaking ?Please see attached patch for my proposal.
Cheers
I think we should check whether fd is NULL or not, otherwise, segmentation
fault maybe occur.+ if (pclose(fd) != 0) + { + write_stderr(_("%s: could not close the file following command \"%s\"\n"), progname, cmd); + exit(1); + }Hi,
That check is a few line above:
+ if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{
Cheers
On Wed, 16 Nov 2022 at 10:02, Japin Li <japinli@hotmail.com> wrote:
I think we should check whether fd is NULL or not, otherwise, segmentation
fault maybe occur.+ if (pclose(fd) != 0) + { + write_stderr(_("%s: could not close the file following command \"%s\"\n"), progname, cmd); + exit(1); + }
Sorry for the noise, I misunderstand it.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Wed, 16 Nov 2022 at 10:06, Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
That check is a few line above:
+ if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{Cheers
Thanks for the explanation. Comment on v2 patch.
fd = popen(cmd, "r");
- if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL || pclose(fd) != 0)
+ if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{
+ pclose(fd);
write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd);
exit(1);
}Here, segfault maybe occurs if fd is NULL. I think we can remove pclose()
safely since the process will exit.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Tue, Nov 15, 2022 at 6:35 PM Japin Li <japinli@hotmail.com> wrote:
On Wed, 16 Nov 2022 at 10:06, Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
That check is a few line above:
+ if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{Cheers
Thanks for the explanation. Comment on v2 patch.
fd = popen(cmd, "r"); - if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL || pclose(fd) != 0) + if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL) { + pclose(fd); write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd); exit(1); }Here, segfault maybe occurs if fd is NULL. I think we can remove pclose()
safely since the process will exit.--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
That means we're going back to v1 of the patch.
Cheers
On Wed, 16 Nov 2022 at 10:52, Ted Yu <yuzhihong@gmail.com> wrote:
On Tue, Nov 15, 2022 at 6:35 PM Japin Li <japinli@hotmail.com> wrote:
fd = popen(cmd, "r"); - if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL || pclose(fd) != 0) + if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL) { + pclose(fd); write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd); exit(1); }Here, segfault maybe occurs if fd is NULL. I think we can remove pclose()
safely since the process will exit.That means we're going back to v1 of the patch.
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we call
pclose() to close fd. The code isn't straightforward, however, it is correct.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Tue, Nov 15, 2022 at 7:12 PM Japin Li <japinli@hotmail.com> wrote:
On Wed, 16 Nov 2022 at 10:52, Ted Yu <yuzhihong@gmail.com> wrote:
On Tue, Nov 15, 2022 at 6:35 PM Japin Li <japinli@hotmail.com> wrote:
fd = popen(cmd, "r");
- if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL||
pclose(fd) != 0) + if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL) { + pclose(fd); write_stderr(_("%s: could not determine the datadirectory
using command \"%s\"\n"), progname, cmd);
exit(1);
}Here, segfault maybe occurs if fd is NULL. I think we can remove
pclose()
safely since the process will exit.
That means we're going back to v1 of the patch.
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we
call
pclose() to close fd. The code isn't straightforward, however, it is
correct.Please read this sentence from my first post:
If the fgets() call doesn't return NULL, the pclose() would be skipped.
On Wed, 16 Nov 2022 at 11:15, Ted Yu <yuzhihong@gmail.com> wrote:
On Tue, Nov 15, 2022 at 7:12 PM Japin Li <japinli@hotmail.com> wrote:
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we
call
pclose() to close fd. The code isn't straightforward, however, it is
correct.Please read this sentence from my first post:
If the fgets() call doesn't return NULL, the pclose() would be skipped.
fgets() returns non-NULL, it means the second condition is false, and
it will check the third condition, which calls pclose(), so it cannot
be skipped, right?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Tue, Nov 15, 2022 at 7:26 PM Japin Li <japinli@hotmail.com> wrote:
On Wed, 16 Nov 2022 at 11:15, Ted Yu <yuzhihong@gmail.com> wrote:
On Tue, Nov 15, 2022 at 7:12 PM Japin Li <japinli@hotmail.com> wrote:
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we
call
pclose() to close fd. The code isn't straightforward, however, it is
correct.Hi,
Please take a look at the following:
https://en.cppreference.com/w/c/io/fgets
Quote: If the failure has been caused by some other error, sets the
*error* indicator
(see ferror() <https://en.cppreference.com/w/c/io/ferror>) on stream. The
contents of the array pointed to by str are indeterminate (it may not even
be null-terminated).
I think we shouldn't assume that the fd doesn't need to be closed when NULL
is returned from fgets().
Cheers
On 16.11.22 04:31, Ted Yu wrote:
On Wed, 16 Nov 2022 at 11:15, Ted Yu <yuzhihong@gmail.com
<mailto:yuzhihong@gmail.com>> wrote:On Tue, Nov 15, 2022 at 7:12 PM Japin Li <japinli@hotmail.com
<mailto:japinli@hotmail.com>> wrote:
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits.
Otherwise, we
call
pclose() to close fd. The code isn't straightforward, however,it is
correct.
Hi,
Please take a look at the following:https://en.cppreference.com/w/c/io/fgets
<https://en.cppreference.com/w/c/io/fgets>
Quote: If the failure has been caused by some other error, sets the
/error/ indicator (see ferror()
<https://en.cppreference.com/w/c/io/ferror>) on |stream|. The contents
of the array pointed to by |str| are indeterminate (it may not even be
null-terminated).
That has nothing to do with the return value of fgets().
On Wed, Nov 16, 2022 at 12:28 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:
On 16.11.22 04:31, Ted Yu wrote:
On Wed, 16 Nov 2022 at 11:15, Ted Yu <yuzhihong@gmail.com
<mailto:yuzhihong@gmail.com>> wrote:On Tue, Nov 15, 2022 at 7:12 PM Japin Li <japinli@hotmail.com
<mailto:japinli@hotmail.com>> wrote:
After some rethinking, I find the origin code do not have
problems.
If fd is NULL or fgets() returns NULL, the process exits.
Otherwise, we
call
pclose() to close fd. The code isn't straightforward, however,it is
correct.
Hi,
Please take a look at the following:https://en.cppreference.com/w/c/io/fgets
<https://en.cppreference.com/w/c/io/fgets>
Quote: If the failure has been caused by some other error, sets the
/error/ indicator (see ferror()
<https://en.cppreference.com/w/c/io/ferror>) on |stream|. The contents
of the array pointed to by |str| are indeterminate (it may not even be
null-terminated).That has nothing to do with the return value of fgets().
Hi, Peter:
Here is how the return value from pclose() is handled in other places:
+ if (pclose_rc != 0)
+ {
+ ereport(ERROR,The above is very easy to understand.
While the check in `adjust_data_dir` is somewhat harder to comprehend.
I think the formation presented in patch v1 aligns with existing checks of
the return value from pclose().
It also gives a unique error message in the case that the return value from
pclose() indicates an error.
Cheers