Check return value of pclose() correctly

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.