Re: closing file in adjust_data_dir

[Ted Yu]

On Wed, Nov 16, 2022 at 12:28 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 16.11.22 04:31, Ted Yu wrote:
> > On Wed, 16 Nov 2022 at 11:15, Ted Yu  > > wrote:
> >  > On Tue, Nov 15, 2022 at 7:12 PM Japin Li  > > 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()
> > ) 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

Re: closing file in adjust_data_dir

[Peter Eisentraut]

On 16.11.22 04:31, Ted Yu wrote:

On Wed, 16 Nov 2022 at 11:15, Ted Yu mailto:yuzhih...@gmail.com>> wrote:
 > On Tue, Nov 15, 2022 at 7:12 PM Japin Li mailto:japi...@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() 
) 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().

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 7:26 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 11:15, Ted Yu  wrote:
> > On Tue, Nov 15, 2022 at 7:12 PM Japin Li  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() ) 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

Re: closing file in adjust_data_dir

[Japin Li]

On Wed, 16 Nov 2022 at 11:15, Ted Yu  wrote:
> On Tue, Nov 15, 2022 at 7:12 PM Japin Li  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.

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 7:12 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 10:52, Ted Yu  wrote:
> > On Tue, Nov 15, 2022 at 6:35 PM Japin Li  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.

Re: closing file in adjust_data_dir

[Japin Li]

On Wed, 16 Nov 2022 at 10:52, Ted Yu  wrote:
> On Tue, Nov 15, 2022 at 6:35 PM Japin Li  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.

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 6:35 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 10:06, Ted Yu  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

Re: closing file in adjust_data_dir

[Japin Li]

On Wed, 16 Nov 2022 at 10:06, Ted Yu  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.

Re: closing file in adjust_data_dir

[Japin Li]

On Wed, 16 Nov 2022 at 10:02, Japin Li  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.

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 6:02 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 02:43, Ted Yu  wrote:
> > Hi,
> > I was looking at the commit:
> >
> > commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
> > Author: Peter Eisentraut 
> > 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

Re: closing file in adjust_data_dir

[Japin Li]

On Wed, 16 Nov 2022 at 02:43, Ted Yu  wrote:
> Hi,
> I was looking at the commit:
>
> commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
> Author: Peter Eisentraut 
> 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.

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 10:43 AM Ted Yu  wrote:

> Hi,
> I was looking at the commit:
>
> commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
> Author: Peter Eisentraut 
> 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


pg-ctl-close-fd-v2.patch
Description: Binary data