Re: kern/58929: POSIX.1-2024 compliance: posix_close, POSIX_CLOSE_RESTART

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Sat, 21 Dec 2024 22:07:19 +0000
    From:        Taylor R Campbell <riastradh%NetBSD.org@localhost>
    Message-ID:  <20241221220723.04ECB84E92%mail.netbsd.org@localhost>

  | Correction: Our current close() -- and probably all existing close()
  | implementations outside buggy proprietary Unix variants -- is not
  | compliant with POSIX.1-2024, because if some underlying I/O is
  | interrupted by a signal it can fail with EINTR even though the
  | descriptor is unconditionally closed.  That is now forbidden (emphasis
  | added):

Can it really?   I haven't looked at our implementation (kernel)
but it used to be the case that even if close() slept for some time,
(like waiting for tty output to drain) and a signal occurred, close()
would still return 0.

But if not, surely the simple change here is to make that happen, not
to work around it in libc - just have sys_close() never return EINTR.

We have to function correctly in this case anyway, as we do close the
fd, so another close(fd) will just return EBADF - so anything draining
still in the kernel output queues needs to be properly handled without
any more userland assistance.

kre
  |
  | > 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] and
  | > fildes _remaining open_, or returns -1 with errno set to
  | > [EINPROGRESS] and fildes being closed, or returns 0 to indicate
  | > successful completion[...]
  | > 
  | > RATIONALE
  | > 
  | > [...] Note that the standard requires that close() and posix_close()
  | > _must leave fildes open after [EINTR]_ (in the cases where [EINTR]
  | > is permitted)
  |
  | So, in order to deal with POSIX.1-2024 having addressed the problem of
  | its own failure to adequately specify semantics by breaking
  | compatibility with every major OS on the planet, it looks like we'll
  | have to do some symbol magic under _POSIX_C_SOURCE -- something like
  | this:
  |
  | /* include/unistd.h */
  |
  | #if (_POSIX_C_SOURCE >= 202408L) && !defined(_NETBSD_SOURCE)
  | int	close(int) __RENAME(__posixly_correct_close);
  | int	posix_close(int, int);
  | #define	POSIX_CLOSE_RESTART	0
  | #else
  | int	close(int);
  | #endif
  |
  | /* lib/libc/sys/posixly_correct_close.c */
  |
  | int
  | __posixly_correct_close(int d)
  | {
  | 	const int ret = close(d);
  |
  | 	if (ret == -1 && errno == EINTR)
  | 		errno = EINPROGRESS;
  |
  | 	return ret;
  | }
  |
  | int
  | posix_close(int d, int flag)
  | {
  | 	const int ret = __posixly_correct_close(d);
  |
  | 	_DIAGASSERT(ret == 0 || errno != EINTR);
  |
  | 	if (ret == 0 && flag != 0) {
  | 		errno = EINVAL;
  | 		ret = -1;
  | 	}
  | 	return ret;
  | }
  |
  | What a colossal waste of effort -- and attack surface for new bugs to
  | crawl onto -- to force everyone else to bend over backwards to
  | accommodate bugs in proprietary implementations of close().
  |