[PATCH] ev: fix epoll_init fd leak

[Jonathan Neuschäfer]

On Wed, Nov 02, 2011 at 12:58:43PM +0100, Christian Parpart wrote:
> On Tue, Nov 1, 2011 at 1:16 AM, Marc Lehmann <schmorp at schmorp.de> wrote:
> 
> > On Mon, Oct 31, 2011 at 09:07:39PM +0100, Jonathan Neuschäfer <
> > j.neuschaefer at gmx.net> wrote:
> > > > 0 is, in fact, not a valid file descriptor in this case, as it is
> > stdin,
> > > > and an epoll fd is not having the properties stdin has - althoguh it'S
> > > > much betterto have an epoll fd there than, say, your shadow file :)
> > >
> > > As this is not exactly the same as checking for a simple error, I think
> > > this line needs a comment why it checks for <= 0 and not < 0.
> >
> > The check for <= 0 is inconsistent and incomplete - libev should either
> > check for <= 2 in all such tests, which gives best coverage of system-fds,
> > or always < 0. Alternatively, it could do what urxvt does and fix up fds
> > 0..2 on init, with warning, as it seems to be a common bug.
> >
> > But this is a quality issue - libev doesn't really give a useful
> > diagnostic in this case, although no harm will be done as it is (libev
> > does not and cannot guarantee atomicity w.r.t. concurrent exec, and libev
> > will work under these conditions).
> >
> > The real issue was and is the broken app that messes with system fds. It
> > needs fixing in any case.
> 
> 
> I do think a little bit different here. Think of a server app. The app:
> 1.) first reads config file,
> 2.) populates internal data structures out of it,
> 3.) cleans up its environment (including closing inherited fds, including
> 0,1,2)
> 4.) initializes its own subsystem, including epoll_create(libev) and its
> own log targets and listener sockets
> 5.) runs the loop until exit.
> 
> This is not a bad behaviour.
> closing all inherited fds right before starting to allocate its intended
> ones is even a really
> sane approach to prevent stale fds from the parent process. (again, we talk
> about a server app).
> now, if the server app would *first* create the log targets (open log
> files, syslog sockets, ...)
> and *then* initializing libev, then libev's epoll_create() invokation is
> fine.
> but dare to do it the other way around (as shown above), then your test
> fails, even though
> epoll_create DID NOT fail. that is a bug in libev, no matter how you turn
> it and the
> app developer has to know that buggy part of libev to not run into it (e.g.
> by making sure fd 0
> is in use, not taking into account fd 1 and 2).

FYI, this inconsistency has been fixed in CVS (patch attached), but
closing fds 0, 1, and 2 can still lead to problems, e.g. when a library
logs an error on standard error (fd 2); so you really have to make sure
your libraries don't do that.
In libev you can only prevent all writes to fd 2, if you overwrite the
system error reporting function (with ev_set_syserr_cb), disable
debugging (libev uses assert to catch some kinds of errors), see the
"ERROR HANDLING" section of the libev manpage for more information.
But I'm not sure you can prevent all other libraries you use from using
the "system" fds.

Thanks,
	Jonathan Neuschäfer

--
Author: root <root>
Date:   Tue Nov 1 00:17:15 2011 +0000

    consistently check for fds, allow programs to mess with sytem fds to some extent (unsupported)

diff --git a/ev_epoll.c b/ev_epoll.c
index 58bf9b9..240b7b9 100644
--- a/ev_epoll.c
+++ b/ev_epoll.c
@@ -238,7 +238,7 @@ epoll_init (EV_P_ int flags)
 #ifdef EPOLL_CLOEXEC
   backend_fd = epoll_create1 (EPOLL_CLOEXEC);
 
-  if (backend_fd <= 0)
+  if (backend_fd < 0)
 #endif
     backend_fd = epoll_create (256);