Nick Mathewson [Mon, 25 Oct 2010 20:00:47 +0000 (16:00 -0400)]
Fix a bug where we would read too much data in HTTP bodies or requests.
We were using evbuffer_add_buffer, which moved the entire buffer
contents. But if we had a valid content_length, we only wanted to
move up to the amount of data remaining in ntoread. Our bug would
make us put our ntoread in the negative, which would in turn make us
read all data until the connection closed.
Nick Mathewson [Mon, 25 Oct 2010 18:29:30 +0000 (14:29 -0400)]
Make evbuffer_add_file take ev_off_t, not off_t
This change has no effect on non-windows platforms, since those
either define off_t to 64-bits, or allow you to decide whether
it should be 64-bits yourself via some LARGEFILE-like macro.
On Windows, however, off_t is always 32-bit, so it's a bad choice
for "file size" or "file offset" values. Instead, I'm adding
an ev_off_t type, and using it in the one place where we used
off_t to mean "the size of a file" or "an offset into a file" in the
API.
Nick Mathewson [Sun, 24 Oct 2010 15:51:14 +0000 (11:51 -0400)]
Simplify the logic for choosing EPOLL_CTL_ADD vs EPOLL_CTL_MOD
Previously, we chose "ADD" whenever old_events==new_events, (since
we expected the add to fail with EEXIST), or whenever old_events
was==0, and MOD otherwise (i.e., when old_events was nonzero and not
equal to new_events).
But now that we retry failed MOD events as ADD *and* failed ADD
events as MOD, the important thing is now to try to guess right the
largest amount of the time, since guessing right means we do only
one syscall, but guessing wrong means we do two.
When old_events is 0, ADD is probably right (unless we're hitting
the dup bug, when we'll fall back).
And when old_events is set and != new_events, MOD is almost
certainly right for the same reasons as before.
But when old_events is equal to new events, then MOD will work fine
unless we closed and reopened the fd, in which case we'll have to
fall back to the ADD case. (Redundant del/add pairs are more common
than closes for most use cases.)
This change lets us avoid calculating new_events, which ought to
save a little time in epoll.c
Nick Mathewson [Sun, 24 Oct 2010 15:38:29 +0000 (11:38 -0400)]
Fix a nasty bug related to use of dup() with epoll on Linux
Current versions of the Linux kernel don't seem to remove the struct
epitem for a given (file,fd) combo when the fd is closed unless the
file itself is also completely closed. This means that if you do:
fd = dup(fd_orig);
add(fd);
close(fd);
dup2(fd_orig, fd);
add(fd);
you will get an EEXIST when you should have gotten a success. This
could cause warnings and dropped events when using dup and epoll.
The solution is pretty simple: when we get an EEXIST from
EPOLL_CTL_ADD, we retry with EPOLL_CTL_MOD.
Unit test included to demonstrate the bug.
Found due to the patient efforts of Gilad Benjamini; diagnosed with
help from Nicholas Marriott.
Nick Mathewson [Wed, 20 Oct 2010 17:41:02 +0000 (13:41 -0400)]
Fix a 100%-CPU bug where an SSL connection would sometimes never stop trying to write
If an SSL connection becamse disabled or suspended before became open,
it could (under the right circumstances) wind up without ever getting
its write callback disabled.
The most correct fix is probably more subtle, and involves checking
all caseswhen a write callback is enabled or disabled. This fix is
more blunt, and explicitly checks whether the callback should have
been disabled at the end of the callback to prevent infinite looping.
Nick Mathewson [Thu, 14 Oct 2010 22:36:07 +0000 (18:36 -0400)]
Increment the version to 2.0.8-rc
NOTE: This is not the official release until I tag it. If you see
this commit, and you decide that Libevent 2.0.8-rc is now
finalized, you might get something besides 2.0.8-rc.
Nick Mathewson [Thu, 14 Oct 2010 14:53:26 +0000 (10:53 -0400)]
Avoid spurious reads from just-created open openssl bufferevents
When handshaking, we listen for reads or writes from the
transport. But when we're connected, we start out with writes enabled
and reads disabled, which means we should not have the transport read
for us.
Nick Mathewson [Fri, 8 Oct 2010 04:59:02 +0000 (00:59 -0400)]
Correct logic on disabling underlying bufferevents when disabling a filter
Previously, whenever writing was disabled on a bufferevent_filter (or
a filtering SSL bufferevent), we would stop writing on the underlying
bufferevent. This would make for trouble, though, since if you
implemented common patterns like "stop writing once data X has been
flushed", your bufferevent filter would disable the underlying
bufferevent after the data was flushed to the underlying bufferevent,
but before actually having it written to the network.
Now, we have filters leave their underlying bufferevents enabled for
reading and writing for reading and writing immediately. They are not
disabled, unless the user wants to disable them, which is now allowed.
To handle the case where we want to choke reading on the underlying
bufferevent because the filter no longer wants to read, we use
bufferevent_suspend_read(). This is analogous to the way that we use
bufferevent_suspend_write() to suspend writing on a filtering
bufferevent when the underlying bufferevent's output buffer has hit
its high watermark.
Nick Mathewson [Tue, 12 Oct 2010 17:46:14 +0000 (13:46 -0400)]
Fix serious bugs in per-bufferevent rate-limiting code
Our old code was too zealous about deleting the refill events that
would actually make connections able to read or write again after
they had run out of bandwidth. Under some circumstances, this could
cause a bufferevent to never actually refill one of its
rate-limiting buckets.
Also, the code treated setting a per-connection rate-limit on a
connection that already had a group-limit as if it were changing the
limit on a connection whose allocation had already run out.
Nick Mathewson [Tue, 12 Oct 2010 16:59:13 +0000 (12:59 -0400)]
Handle rate-limiting for reading on OpenSSL bufferevents correctly.
We were looking at the number of bytes read on the wbio, not in the
rbio. But these are usually different BIOs, and the reading is
supposed to happen on the rbio.
Nick Mathewson [Fri, 8 Oct 2010 17:05:13 +0000 (13:05 -0400)]
New evhttp_uri(encode|decode) functions to handle + and NUL characters right
The old evhttp_decode_uri() function would act as tough it was doing
an (illegal, undefined) decode operation on a whole URL at once, and
treat + characters following a ? as different from + characters
preceding one. But that's not useful: If you are decoding a URI
before splitting off query parameters, you are begging to fail as soon
as somebody gives you a value with an encoded & in it.
The new evhttp_uridecode() function takes an argument that says
whether to decode + signs. Both uridecode and uriencode also now
support encoding or decoding to strings with internal 0-valued
characters.
Nick Mathewson [Fri, 8 Oct 2010 16:57:11 +0000 (12:57 -0400)]
evhttp_encode_uri encodes all reserved characters, including !$'()*+,/:=@
Perviously, some characters not listed as "unreserved" by RFC 3986
(notably "!$'()*+,/:=@") were not encoded by evhttp_encode_uri. This
made trouble, especially when encoding path components (where @ and /
are bad news) and parameters (where + should get encoded so it doesn't
later decode into a space).
Nick Mathewson [Wed, 6 Oct 2010 15:48:52 +0000 (11:48 -0400)]
Let evhttp_parse_query return -1 on failure
We already detected certain malformed queries, but we responded by
aborting the query-parsing process half-way through without telling
the user. Now, if query-parsing fails, no headers are returned, and
evhttp_parse_query returns -1.
Nick Mathewson [Wed, 6 Oct 2010 01:34:07 +0000 (21:34 -0400)]
Fix an EINVAL on evbuffer_write_iovec on OpenSolaris.
The writev() call is limited to at most IOV_MAX iovecs (or UIO_MAXIOV,
depending on whom you ask). This isn't a problem anywhere we've
tested except on OpenSolaris, where IOV_MAX was a mere 16.
This patch makes us go from "use up to 128 iovecs when writing" to
"use up to 128 iovecs when writing, or IOV_MAX/UIO_MAXIOV, whichever
is less". This is still wrong if you somehow find a platform that
defines IOV_MAX < UIO_MAXIOV, but I hereby claim that such a platform
is too stupid to worry about for now.
Nick Mathewson [Fri, 1 Oct 2010 03:15:47 +0000 (23:15 -0400)]
Fix a spurious-call bug on epoll.c
We were trying to check whether any events had really been
notified on an fd before calling evmap_io_active on it, but instead
we were checking for an event pointer, which was always true.
In practice, this patch shouldn't change much, since epoll_wait
shouldn't return an event unless there is actually an event going
on.
Spotted by an anonymous bug reporter on Sourceforge. Closes bug 3078425.
Nick Mathewson [Fri, 24 Sep 2010 02:45:55 +0000 (22:45 -0400)]
Fix all warnings in the main codebase flagged by -Wsigned-compare
Remember, the code
int is_less_than(int a, unsigned b) {
return a < b;
}
is buggy, since the C integer promotion rules basically turn it into
int is_less_than(int a, unsigned b) {
return ((unsigned)a) < b;
}
and we really want something closer to
int is_less_than(int a, unsigned b) {
return a < 0 || ((unsigned)a) < b;
}
.
Nick Mathewson [Wed, 15 Sep 2010 05:40:02 +0000 (01:40 -0400)]
Make default signal backend fully threadsafe
Jason Toffaletti discovered with helgrind that our signal handler was
messing with evsig_base, which can be set from lots of places in the
code. Ordinarly, we'd just stick a lock on it, except that it is
illegal (and genuinely error-prone) to call pthread_mutex_acquire()
from inside a signal handler.
The solution is to only store the fd we write to in a static variable,
write the signal number to the fd, and put evsig_cb in charge of
activating signal events.
I have no idea how we'll cope if we want to enable this to handle
siginfo (where available) in the future.
Nick Mathewson [Wed, 15 Sep 2010 05:08:39 +0000 (01:08 -0400)]
Warn when using the error-prone EV_SIGNAL interface in an error-prone way. Also, fix a couple of race conditions in signal.c
When using the signal.c signal backend, Libevent currently only allows
one event_base to actually receive signals at a time. (This has been
the behavior since at least 1.4 and probably much earlier.) Now, we
detect and warn if you're likely to be racing about which signal goes
to which thread.
We also add a lock to control modifications of the evsig_base field,
to avoid race conditions like those found by Jason Toffaletti.
Nick Mathewson [Wed, 8 Sep 2010 17:22:55 +0000 (13:22 -0400)]
Minimize calls to base_notify implementation functions, thereby avoiding needless syscalls
The trick here is that if we already told the base to wake up, and it
hasn't woken up yet, we don't need to tell it to wake up again. This
should help lots with inherently multithreaded code like IOCP.