[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160504122336.GA14517@notk.org>
Date: Wed, 4 May 2016 14:23:36 +0200
From: Adrien Nader <adrien@...k.org>
To: oss-security@...ts.openwall.com
Cc: David Moreno Montero <dmoreno@...albits.com>,
Zachary Grafton <zachary.grafton@...il.com>,
Remi Birot-Delrue <asgeir@...e.fr>
Subject: Re: libonion 0.8 contains security fixes
Hi,
On Wed, May 04, 2016, Solar Designer wrote:
> In particular, when the library is built with Linux epoll support and is
> used in the pool of threads mode, as invoked with onion_new(O_POOL),
> there was a file descriptor and data race condition between handling of
> epoll events and timeout events.>
> [...]
This kind of issue is not limited to multi-threading: I've encountered
the same thing (in another, non-free, product) because file descriptors
were dup'ed() in order to ease management and were then passed to epoll.
Doing this is probably a typical use of epoll. In man 7 epoll, the second
question in the "Question and answers" section is:
What happens if you register the same file descriptor on an epoll
instance twice?
You will probably get EEXIST. However, it is possible to add a
duplicate (dup(2), dup2(2), fcntl(2) F_DUPFD) file descriptor to
the same epoll instance. This can be a useful technique for fil‐
tering events, if the duplicate file descriptors are registered
with different events masks.
In my case, one fd was used to wait on input operations and then dup'ed
to be used for output operations. Otherwise we had to remember which
flags had been set when we wanted to stop getting events about
writeability (i.e. "was EPOLLIN also set?") and the logic was much more
complex in the end.
However, epoll_wait() can return events for both file descriptors in a
single call and it is possible to free the associated data because of
the first event, do some other work and then handle the work for the
dup'ed file descriptor.
The code isn't using one-shot and the fix was to invalidate the event
set upon closing any connection, stop treating the current event batch
and call epoll_wait() again (remember: not one-shot mode so no event is
lost).
> commit 4e111f30c1adf0ba0d5814a24f904dea35310a37
> Author: Solar Designer <solar@...nwall.com>
> Date: Sun Mar 27 19:16:04 2016 +0300
>
> Complete the implementation of Rich Felker's idea (partially introduced
> with commit c80c46d5ff842291f0cce3917e7b8340c43d4315) to shutdown()
> rather than close() on timeout, so that the fd is held until after
> another one-shot epoll event arrives. This way, we don't close the fd
> (thereby not freeing it for possible reuse just yet) and don't free the
> slot asynchronously to a possible event for the same slot on another
> thread. When we do receive another event for the shutdown() fd, we
> expect that no concurrent event is being processed for the same fd and
> the same slot due to the one-shot property of the epoll instance.
> In other words, we postpone the potential fd and slot memory reuse until
> after we're out of the asynchronous timeout handling and into the
> per-slot synchronous one-shot event handling.
>
> Handle timeouts in busy servers more optimally: check for them once per
> second (and per thread for now) rather than once per event.
I've also found myself calling shutdown() simply so that I could
receive an error event and could then close everything, albeit in a
slightly different context.
I'm wondering if this is a common practice. I hadn't read about it I
think and I'm once again wondering how many people do this. Also, does
anyone know of more "usage tricks" than what is in man 7 epoll?
--
Adrien Nader
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
