Re: some fixes to musl

[Rich Felker <dalias@aerifal.cx>]

On Thu, Jul 21, 2011 at 09:02:55PM +0400, Vasiliy Kulikov wrote:
> Hi,
> 
> This is a patch against v0.7.12.  It is only compile tested.
> 
> fdopendir():
> - POSIX defines EBADF if fd is invalid.  I suppose it is OK
>   to assume fd is invalid if fstat() failed.

Fixed.

> - fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
>   fails, the consequences are not expected.

Also, per POSIX:

  It is unspecified whether the FD_CLOEXEC flag will be set on the
  file descriptor by a successful call to fdopendir().

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html

> rewinddir(), seekdir():
> - lseek(fd, 0, SEEK_SET) may fail too, I suppose it can be true for
>   network file systems or FUSE fses.  Continuing the work with pos=-1 is
>   somewhat not expected.

I've checked all code that uses the position and it doesn't matter.
The DIR buffer will be empty so the next readdir call will invoke
getdents and get an error again.

> cuserid():
> - POSIX says an argument may be NULL, in this case the buffer should be
>   statically allocated.

This function was removed in SUSv3 and was legacy even in v2. Also
historical versions of the function differed in behavior...

> forkpty():
> - It should be guaranteed that master fd is closed, tty is setup, slave
>   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
>   rlimit.  setsid() is checked for company :)  I think the only way to
>   handle the failure is _exit().  While it may be not the best choise,
>   however, continuing the work with half dropped privileges is more
>   dangerous.
> 
> openpty():
> - close() shouldn't change errno updated by failed ioctl()/open().
> - I suppose the last calls to tcsetattr() and ioctl() may fail too.

Going to try to find a good solution for these...

> _vsyslog():
> - sendto() may fail in case of signal delivery.

I'm opting not to change this one. EINTR can only happen if the
application intentionally installed an interrupting signal handler,
and in this case, the author probably intended to be able to interrupt
any blocking operation. For instance consider the typical use of
alarm(). With your proposed change, syslog would block forever in the
socket buffers were full (syslog daemon hung) and an application
aiming for hardcore robustness/realtime use could not recover.

As-is, the worst that could happen is log messages being lost, but
this will only happen in applications where the programmer has
explicitly asked for interrupting signals.

Also note that the loop-and-retry code was wrong because the socket is
a datagram socket. Retrying mid-buffer would send a new malformed
datagram to the syslog daemon...

> I wonder what is the Right Way to handle close() failures in generic
> case.  If it is fork'ish function, _exit() can be used.  If it is some
> privilege dropping function, failure can be returned in errno, but the
> task state (hanged/unclosed fd) is not expected by the caller.

And thus probably unrecoverable, since the caller does not know the
file descriptor number you failed to close...

> More dangerous case is function that is not designed to return error at
> all.  E.g. close() inside of closelog() may fail, but the caller cannot
> be notified about it by design.  If the caller want to do chroot(),
> which prevents re-opening log fd, the failure would break task's
> expectation of dropped privileges (closed log fd).

My view is that a system aiming at production quality must ensure that
close cannot fail for valid file descriptors. The ability of close to
fail will lead to unrecoverable errors in any program that uses
stdio/fclose, because:

1. The fclose() function shall perform the equivalent of a close() on
the file descriptor that is associated with the stream pointed to by
stream.

2. After the call to fclose(), any use of stream results in undefined
behavior.

If close can fail, then fclose can fail for the same reasons, making
the FILE that refers to the open file unusable, but leaving the file
descriptor in limbo. This is a lot like the issue you mentioned with
closelog, except it affects 99% of applications rather than 1-5% of
applications.

Rich