Thread: Check return value of pclose() correctly

I noticed that some (not all) callers didn't check the return value of 
pclose() or ClosePipeStream() correctly.  Either they didn't check it at 
all or they treated it like the return of fclose().  Here is a patch 
with fixes.

(A failure to run the command issued by popen() is usually reported via 
the pclose() status, so while you can often get away with not checking 
fclose() or close(), checking pclose() is more often useful.)

There are some places where the return value is apparently intentionally 
ignored, such as in error recovery paths, or psql ignoring a failure to 
launch the pager.  (The intention can usually be inferred by the kind of 
error checking attached to the corresponding popen() call.)  But there 
are a few places in psql that I'm suspicious about that I have marked, 
but need to think about further.

On Mon, Oct 31, 2022 at 09:12:53AM +0100, Peter Eisentraut wrote:
> I noticed that some (not all) callers didn't check the return value of
> pclose() or ClosePipeStream() correctly.  Either they didn't check it at all
> or they treated it like the return of fclose().  Here is a patch with fixes.
>
> (A failure to run the command issued by popen() is usually reported via the
> pclose() status, so while you can often get away with not checking fclose()
> or close(), checking pclose() is more often useful.)

-   if (WIFEXITED(exitstatus))
+   if (exitstatus == -1)
+   {
+       snprintf(str, sizeof(str), "%m");
+   }
This addition in wait_result_to_str() looks inconsistent with the
existing callers of pclose() and ClosePipeStream() that check for -1
as exit status.  copyfrom.c and basebackup_to_shell.c fall into this
category.  Wouldn't it be better to unify everything?

> There are some places where the return value is apparently intentionally
> ignored, such as in error recovery paths, or psql ignoring a failure to
> launch the pager.  (The intention can usually be inferred by the kind of
> error checking attached to the corresponding popen() call.)  But there are a
> few places in psql that I'm suspicious about that I have marked, but need to
> think about further.

Hmm.  I would leave these out, I think.  setQFout() relies on the
result of openQueryOutputFile().  And this could make commands like
\watch less reliable.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Oct 31, 2022 at 09:12:53AM +0100, Peter Eisentraut wrote:
>> (A failure to run the command issued by popen() is usually reported via the
>> pclose() status, so while you can often get away with not checking fclose()
>> or close(), checking pclose() is more often useful.)
 
> -   if (WIFEXITED(exitstatus))
> +   if (exitstatus == -1)
> +   {
> +       snprintf(str, sizeof(str), "%m");
> +   }
> This addition in wait_result_to_str() looks inconsistent with the 
> existing callers of pclose() and ClosePipeStream() that check for -1
> as exit status.  copyfrom.c and basebackup_to_shell.c fall into this
> category.  Wouldn't it be better to unify everything?

I think there are two issues here.  POSIX says

    Upon successful return, pclose() shall return the termination status
    of the command language interpreter. Otherwise, pclose() shall return
    -1 and set errno to indicate the error.

That is, first you need to make sure that pclose returned a valid
child process status, and then you need to decode that status.
It's not obvious to me that -1 is disjoint from the set of possible
child statuses.  Do we need to add some logic that clears and then
checks errno?

Also, we have a number of places --- at least FreeDesc() and
ClosePipeStream() --- that consider pclose()'s return value to be
perfectly equivalent to that of close() etc, because they'll
return either one without telling the caller which is which.
It seems like we have to fix that if we want to issue sane
error reports.

This patch isn't moving things forward on this fundamental
confusion.

            regards, tom lane

On 01.11.22 06:35, Michael Paquier wrote:
> -   if (WIFEXITED(exitstatus))
> +   if (exitstatus == -1)
> +   {
> +       snprintf(str, sizeof(str), "%m");
> +   }
> This addition in wait_result_to_str() looks inconsistent with the
> existing callers of pclose() and ClosePipeStream() that check for -1
> as exit status.  copyfrom.c and basebackup_to_shell.c fall into this
> category.  Wouldn't it be better to unify everything?

With the above addition, the extra check for -1 at those existing places 
could be removed.

>> There are some places where the return value is apparently intentionally
>> ignored, such as in error recovery paths, or psql ignoring a failure to
>> launch the pager.  (The intention can usually be inferred by the kind of
>> error checking attached to the corresponding popen() call.)  But there are a
>> few places in psql that I'm suspicious about that I have marked, but need to
>> think about further.
> 
> Hmm.  I would leave these out, I think.  setQFout() relies on the
> result of openQueryOutputFile().  And this could make commands like
> \watch less reliable.

I don't quite understand what you are saying here.  My point is that, 
for example, setQFout() thinks it's important to check the result of 
popen() and write an error message, but it doesn't check the result of 
pclose() at all.  I don't think that makes sense in practice.

On 01.11.22 06:52, Tom Lane wrote:
> I think there are two issues here.  POSIX says
> 
>      Upon successful return, pclose() shall return the termination status
>      of the command language interpreter. Otherwise, pclose() shall return
>      -1 and set errno to indicate the error.
> 
> That is, first you need to make sure that pclose returned a valid
> child process status, and then you need to decode that status.
> It's not obvious to me that -1 is disjoint from the set of possible
> child statuses.  Do we need to add some logic that clears and then
> checks errno?

This return convention is also used by system() and is widely used.  So 
I don't think we need to be concerned about this.

In practice, int is 4 bytes and WEXITSTATUS() and WTERMSIG() are one 
byte each, so they are probably in the lower bytes, and wouldn't 
accidentally make up a -1.

> Also, we have a number of places --- at least FreeDesc() and
> ClosePipeStream() --- that consider pclose()'s return value to be
> perfectly equivalent to that of close() etc, because they'll
> return either one without telling the caller which is which.
> It seems like we have to fix that if we want to issue sane
> error reports.

I think this works.  FreeDesc() returns the pclose() exit status to 
ClosePipeStream(), which returns it directly.  No interpretation is done 
within these functions.

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Hi Peter,
This is a review of the pclose return value check patch:

Contents & Purpose:
Purpose of this patch is to properly handle return value of pclose (indirectly, return from ClosePipeStream).

Initial Run:
The patch applies cleanly to HEAD. The regression tests all pass
successfully against the new patch.

Conclusion:
At some places pclose return value is handled and this patch adds return value check for remaining values. 
Implementation is in sync with existing handling of pclose.

- Ankit K Pandey

On 01.11.22 21:30, Peter Eisentraut wrote:
>>> There are some places where the return value is apparently intentionally
>>> ignored, such as in error recovery paths, or psql ignoring a failure to
>>> launch the pager.  (The intention can usually be inferred by the kind of
>>> error checking attached to the corresponding popen() call.)  But 
>>> there are a
>>> few places in psql that I'm suspicious about that I have marked, but 
>>> need to
>>> think about further.
>>
>> Hmm.  I would leave these out, I think.  setQFout() relies on the
>> result of openQueryOutputFile().  And this could make commands like
>> \watch less reliable.
> 
> I don't quite understand what you are saying here.  My point is that, 
> for example, setQFout() thinks it's important to check the result of 
> popen() and write an error message, but it doesn't check the result of 
> pclose() at all.  I don't think that makes sense in practice.

I have looked this over again.  In these cases, if the piped-to command 
is faulty, you get a "broken pipe" error anyway, so the effect of not 
checking the pclose() result is negligible.  So I have removed the 
"FIXME" markers without further action.

There is also the question whether we want to check the exit status of a 
user-supplied command, such as in pgbench's \setshell.  I have dialed 
back my patch there, since I don't know what the current practice in 
pgbench scripts is.  If the command fails badly, pgbench will probably 
complain anyway about invalid output.

More important is that something like pg_upgrade does check the exit 
status when it calls pg_controldata etc.  That's what this patch 
accomplishes.

On 02.11.22 16:26, Ankit Kumar Pandey wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> Hi Peter,
> This is a review of the pclose return value check patch:
> 
> Contents & Purpose:
> Purpose of this patch is to properly handle return value of pclose (indirectly, return from ClosePipeStream).
> 
> Initial Run:
> The patch applies cleanly to HEAD. The regression tests all pass
> successfully against the new patch.
> 
> Conclusion:
> At some places pclose return value is handled and this patch adds return value check for remaining values.
> Implementation is in sync with existing handling of pclose.

Committed.  Thanks for the review.