[PATCH] Manual should discourage mixing TEMP_FAILURE_RETRY and close

[Michael Kerrisk]

For what it's worth, well after the fact...

On Wed, Dec 4, 2013 at 1:08 AM, Ondřej Bílka <neleai@seznam.cz> wrote:
> On Tue, Dec 03, 2013 at 01:05:21PM -0500, Mark Mentovai wrote:
>> When the close system call is interrupted by a signal, the state of its
>> file descriptor argument is not specified[1]. On Linux, a close that
>> fails with EINTR must not be restarted because the file descriptor is
>> guaranteed to have been closed at that point[2]. Note that the kernel
>> itself never restarts a close when SA_RESTART is set or when no
>> user-space signal handler is present[3].
>>
> This is linux problem, POSIX now says
>
> http://austingroupbugs.net/view.php?id=529
>
> If close( ) is interrupted by a signal that is to be caught, then it
> is unspecified whether it returns -1 with errno set to [EINTR] with
> fildes remaining open, or returns -1 with errno set to [EINPROGRESS]
> with fildes being closed, or returns 0 to indicate successful
> completion; except that if POSIX_CLOSE_RESTART is defined as 0, then
> the option of returning -1 with errno set to [EINTR] and fildes
> remaining open shall not occur. If close() returns -1 with errno set
> to [EINTR], it is unspecified whether fildes can subsequently be
> passed to any function except close( ) or posix_close( ) without error.
> For all other error situations (except for [EBADF] where fildes was
> invalid), fildes shall be closed. If fildes was closed even though
> the close operation is incomplete, the close operation shall continue
> asynchronously and the process shall have no further ability to track
> the completion or final status of the close operation.

Yes, but POSIX never said that. That was a proposal that was
discussed, that subsequently got shot down... Older POSIX.1 was wrong.
It's worth reading that entire Austin bug for the subsequent
discussion.

POSIX.1-2016 says under close()

       If  close()  is  interrupted  by a signal that is to be caught, it
       shall return −1 with errno set to [EINTR] and the state of  fildes
       is  unspecified.  If  an  I/O error occurred while reading from or
       writing to the file system during close(), it may return  −1  with
       errno set to [EIO]; if this error is returned, the state of fildes
       is unspecified.

(The only three errors mandated in POSIX.1-2016 for close() are EINTR,
EIO, and EBADF.)

My takeaway from the above POSIX text is that it's perfectly
legitimate for an implementation to say that  retrying close() after
*any* error is the wrong thing to do. Linux does this. So does
FreeBSD, where the man page says:

     In case of any error except EBADF, the supplied file descriptor is
     deallocated and therefore is no longer valid.

And according to a claim in the thread here:
https://news.ycombinator.com/item?id=3363884 , ancient AIX also
explicitly documented that the FD was closed in an EINTR error was
returned. This looks to be confirmed in
http://publib16.boulder.ibm.com/doc_link/en_US/a_doc_lib/libs/basetrf1/close.htm

And from the look of http://austingroupbugs.net/view.php?id=529#c1200
, the plan in the future is that POSIX will allow even more
flexibility to an implementation in the next major release (who knows
when).

>> Because glibc is widely used on Linux, its documentation should state
>> affirmatively that the TEMP_FAILURE_RETRY, which restarts interrupted
>> system calls that fail with EINTR, is not to be used with close on
>> Linux. Examples in glibc documentation should avoid recommending
>> wrapping close with TEMP_FAILURE_RETRY.
>>
>> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html
>> [2] http://lkml.org/lkml/2005/9/10/129
>> [3] http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=ee731f4f7880b09ca147008ab46ad4e5f72cb8b
>>
>> 2013-12-03  Mark Mentovai  <mark@chromium.org>
>>
>>       * manual/llio.texi (Opening and Closing Files): Document that
>>       TEMP_FAILURE_RETRY should not be used with close on Linux.
>>       (Duplicating Descriptors): Don't wrap close in TEMP_FAILURE_RETRY
>>       in example code.

This patch of Mark's really should be applied in some form, I think.
The glibc manual is currently advising users to do something actively
dangerous on at least two popular systems, and it has no justification
in current POSIX.

I'm just in the process of beefing up the Linux close man page on this
subject to say:

       Not checking the return value of close() is a common but neverthe‐
       less  serious programming error.  It is quite possible that errors
       on a previous write(2) operation are first reported at  the  final
       close().   Not checking the return value when closing the file may
       lead to silent loss of data.  This can especially be observed with
       NFS and with disk quota.

       Note, however, that the return value should be used only for diag‐
       nostics.  Retrying the close() after an error is the  wrong  thing
       to  do, since this may cause a reused file descriptor from another
       thread to be closed.  (The kernel  releases  the  file  descriptor
       early in the close operation, freeing it for reuse; the steps that
       may return an error, such as flushing data  to  disk,  occur  only
       later  in the close operation.)  In particular, close() should not
       be retried after an EINTR error.  (POSIX.1 says:  "If  close()  is
       interrupted  by  a signal that is to be caught, it shall return -1
       with errno set to EINTR and the state of fildes is unspecified.")

       Careful users who want  to  know  about  I/O  errors  may  precede
       close() with a call to fsync(2).

Cheers,

Michael

>>
>> ---
>>  manual/llio.texi | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/manual/llio.texi b/manual/llio.texi
>> index b6c9260..f04a2cf 100644
>> --- a/manual/llio.texi
>> +++ b/manual/llio.texi
>> @@ -253,11 +253,9 @@ The @var{filedes} argument is not a valid file descriptor.
>>  @item EINTR
>>  The @code{close} call was interrupted by a signal.
>>  @xref{Interrupted Primitives}.
>> -Here is an example of how to handle @code{EINTR} properly:
>> -
>> -@smallexample
>> -TEMP_FAILURE_RETRY (close (desc));
>> -@end smallexample
>> +On Linux, @code{close} will have closed its file descriptor argument even when
>> +reporting @code{EINTR}.  It is not safe to retry calling @code{close} in this
>> +case, as would be done when wrapped in the @code{TEMP_FAILURE_RETRY} macro.
>>
>>  @item ENOSPC
>>  @itemx EIO
>> @@ -2765,7 +2763,7 @@ if (pid == 0)
>>      @dots{}
>>      file = TEMP_FAILURE_RETRY (open (filename, O_RDONLY));
>>      dup2 (file, STDIN_FILENO);
>> -    TEMP_FAILURE_RETRY (close (file));
>> +    close (file);
>>      execv (program, NULL);
>>    @}
>>  @end smallexample
>> --
>> 1.8.3.4
>