Thread: closing file in adjust_data_dir

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