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.

Thanks. Can you look over my below review of the issues and see if you
agree on what's actually an issue and what's not. I like to avoid
tinfoil-hat programming -- that is, testing for error conditions from
interfaces that have no standard reason to fail when used in the way
they're being used, and for which it would be a very poor
quality-of-implementation issue for a kernel implementation to add new
implementation-specific failure conditions.

I'm aware of course that some interfaces *can* fail for nonstandard
reasons under Linux (dup2 and set*uid come to mind), and I've tried to
work around these and shield applications from the brokenness...

> fdopendir():
> - POSIX defines EBADF if fd is invalid.  I suppose it is OK
>   to assume fd is invalid if fstat() failed.

fstat will already set errno to EBADF if appropriate. In that case we
just need to avoid overwriting it. I'll fix that by separating failure
into two conditionals.

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

The only way it can fail is if fd is invalid, which was already tested
by fstat. If another thread or signal handler closes fd here and
causes a race condition, behavior is going to be unpredictable
(undefined) anyway.

> 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'm not sure what could be meaningfully done in that case... I would
actually consider it a broken fs driver if lseek to 0 ever fails on a
directory but it's worth investigating if it's a real issue.

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

I'll check it out.

> 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.

The only way it could fail is if one of these fds was not open to
begin with, which would be pretty unusual. It's unclear to me whether
it's better to fail early or fail later.. unfortunately there's no
good way to report failure to the caller after fork succeeds. Perhaps
I could attempt fcntl F_DUPFD on the slave pty in the parent process
before forking to reserve fd slots 0-2 if they're not already taken...

>  setsid() is checked for company :)  I think the only way to

I'm pretty sure setsid cannot fail. There's no justifiable reason for
failure. POSIX says:

  [EPERM] The calling process is already a process group leader, or
  the process group ID of a process other than the calling process
  matches the process ID of the calling process.

So supposedly it can fail if the process id of an old process what was
previously a process group leader got reused while some processes in
the old process group had not yet terminated. I would contend that a
quality implementation would not allow a process id to be reassigned
if any processes still have that id as their process group id. Do you
know if Linux satisfies this condition?

>   handle the failure is _exit().  While it may be not the best choise,
>   however, continuing the work with half dropped privileges is more
>   dangerous.

Can you clarify what you mean about "dropped privileges"?

> openpty():
> - close() shouldn't change errno updated by failed ioctl()/open().

This call to close cannot fail short of another thread or signal
handler poking at file desciptor numbers that don't belong to it.

> - I suppose the last calls to tcsetattr() and ioctl() may fail too.

Indeed, and this is testable/reportable. I suppose it would be best to
mimic what existing implementations do here.

> realpath() (no patch):

This function is just a hack to stand in until I write a real version
of it. See the commit message. I agree totally that it's wrong.

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

This should probably be tested and retried since syslog cannot report
errors to the caller.

> I wonder what is the Right Way to handle close() failures in generic
> case.

I think 99% of them are complete non-issues. The whole idea that close
can fail (for reasons other than EBADF) is a misdesign, but it can
generally only happen for odd devices or NFS (which is broken in many
more fundamental ways anyway). It certainly can't fail for terminal
devices, pipes, sockets, directories, etc. which are the only places
libc needs it.

>  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.
> 
> 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).

Non-issue. close cannot fail on a local unix socket.

Rich