[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110721191643.GA29045@albatros>
Date: Thu, 21 Jul 2011 23:17:04 +0400
From: Vasiliy Kulikov <segoon@...nwall.com>
To: musl@...ts.openwall.com
Subject: Re: some fixes to musl
On Thu, Jul 21, 2011 at 14:21 -0400, Rich Felker wrote:
> > 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.
I think it's wrong. *stat(2) calls LSM hook, which may fail and return
arbitrary error. I wish there were *explicit* rules of LSMs what
restrictions hooks have got. But I don't know any similar documentation
:( So, I assume the theoretically worst case - any LSM hook may return
arbitrary error. Yes, it might break many implicit assumptions, etc.
etc. But AFAIU, POSIX and other standards define a set of error that
might be used by implementations, leaving implementations free to
define their own errors. So, strictly speaking, fstat() returning smth
like ENOMEM is not POSIX violation. IMO LSM policy and error
handling code should have as little interconnection as possible, so the
code should assume almost every syscall can fail for "another reason"
without any thinking like "wtf, how this syscall could fail?!?".
> 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.
The same here, fcntl() has LSM hook.
> > 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.
I don't think you can do anything meaningful here. I was thinking about
negative ->tell would lead to some buffer overflow. Looking into the
code I see it is not used internally it musl, so checking ->tell doesn't
help much. => This part of the patch is indeed meaningless.
> > 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.
But possible. Also POSIX defines EINTR error in dup2(), but you handle
EBUSY only.
> 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...
This is a good idea.
> > 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.
Looks like Linux follows POSIX here.
> > 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"?
Here privilege dropping was close() with master fd and closing 0,1,2 fds
if they were opened.
> > - 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.
My 2 cents here - glibc is not the best example to mimic. It doesn't
check some syscalls that I would check (where it is really simple to
introduce a check).
> > 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.
Maybe. I'm worried for fd leaking only, when a process drops some
privileges leaving some fd in half working state opened. Indeed, it
might be not a musl case.
Thanks,
--
Vasiliy
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.
