closing file in adjust_data_dir

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 +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

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 +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

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 +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

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 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.

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&gt;) 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&gt;
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&gt;) 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&gt;
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&gt;) 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