Feedback-ID: i131946ab:Fastmail
Message-ID: <175ef638-32c7-1a84-590c-6df7497baeea@enterprisedb.com>
Date: Tue, 1 Nov 2022 16:41:15 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.1
Subject: Re: Check return value of pclose() correctly
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com>
 <Y2CwITuzlNpF/dWj@paquier.xyz> <1091330.1667281922@sss.pgh.pa.us>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <1091330.1667281922@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/175ef638-32c7-1a84-590c-6df7497baeea%40enterprisedb.com>
Precedence: bulk

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.