Date: Thu, 21 Jul 2011 14:21:01 -0400 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Subject: Re: some fixes to musl 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.