Enji Cooper [Mon, 25 Feb 2019 19:59:15 +0000 (11:59 -0800)]
Define `_GNU_SOURCE` properly/consistently per autoconf
Although `_GNU_SOURCE` can be defined as an arbitrary #define per the
glibc docs [1], it's best to define it in a manner consistent with the way
that autoconf defines it, i.e., `1`.
While this shouldn't matter in most cases, it does when the headers from
other projects follow the poorly defined GNU convention implemented by
autoconf and are included after the libevent's util.h header. An example
failure with clang, similar to the failure I encountered, is as follows:
```
$ printf "#define _GNU_SOURCE\n#define _GNU_SOURCE 1" | clang -c -x c -
<stdin>:2:9: warning: '_GNU_SOURCE' macro redefined [-Wmacro-redefined]
^
<stdin>:1:9: note: previous definition is here
^
1 warning generated.
```
This happened when compiling python [2] with a stale homebrew util.h file from
libevent (which admittedly would not happen in a correct libevent install, as the
header should be installed under /usr/local/include/event2/util.h). However, if
both headers had been combined (which is more likely), it would have failed as
shown above.
Removing the ad hoc definition unbreaks compiling python's pyconfig.h.in header
when included after util.h from libevent.
Azat Khuzhin [Sun, 24 Feb 2019 14:07:18 +0000 (17:07 +0300)]
Use kill() over raise() for raising the signal (fixes osx 10.14 with kqueue)
On OSX 10.14+ the raise() uses pthread_kill() (verified with dtruss) and
by some reason signals that has been raised with pthread_kill() do not
received by kqueue EVFILT_SIGNAL.
While on OSX 10.11 the raise()/pthread_kill() uses plain kill() and
everything work just fine (linux also does the same, but instead of
kill() it uses tgkill())
Here is a simple reproducer that installs alarm to show that the signal
does not received by the kqueue backend:
https://gist.github.com/azat/73638b8e3b0fa563a20dadcca9e652a1
Azat Khuzhin [Sun, 3 Feb 2019 15:47:14 +0000 (18:47 +0300)]
test/ssl/bufferevent_wm: explicitly break the loop once client/server received enough
There can be tricky cases (that can be reproduced by reducing
SO_RCVBUF/SO_SNDBUF to 6144, on linux, and be aware, since linux doubles
this const), when there is still write event pending, although we read
enough.
This should be fixed in a more sophisticated way, but to backport the
patch, let's simply break the loop manually.
The ssl/bufferevent_wm originally failed on solaris.
Azat Khuzhin [Tue, 29 Jan 2019 18:06:37 +0000 (21:06 +0300)]
test/et/et: use evutil_socket_t* over int* for pointer to the pair
Next code will not work correctly under win x64:
evutil_socket_t very_long_pair_name[2];
int *pair = very_long_pair_name; // <-- accessing the second word of the first element
Because sizeof(evutil_socket_t) == sizeof(intptr_t) == 8
P.S. in the 5334762f another test had been fixed instead of the one that
really fails.
Fixes: 5334762f ("test/et/et: fix it by using appropriate type for the SOCKET (evutil_socket_t)")
Refs: #750
(cherry picked from commit 0791a17204ff70bbea92520352a0c6e8d185fa4b)
Azat Khuzhin [Tue, 29 Jan 2019 07:54:39 +0000 (10:54 +0300)]
Merge branch 'win64-fixes'
* win64-fixes:
test/et/et: fix it by using appropriate type for the SOCKET (evutil_socket_t)
test/et/et: verify return codes
appveyor: switch to new VS/MinGW and x64
* http-EVHTTP_CON_READ_ON_WRITE_ERROR-fixes-v2:
http: try to read existing data in buffer under EVHTTP_CON_READ_ON_WRITE_ERROR
test: add logging for http/read_on_write_error and rearrange code
http: do not call deferred readcb if readcb is not set
Azat Khuzhin [Sat, 26 Jan 2019 15:52:33 +0000 (18:52 +0300)]
test: adjust expecting error for getaddrinfo() under EMFILE
When getaddrinfo() cannot allocate file descriptor glibc/musl-libc on
linux report EAI_SYSTEM error. But this is not true for freebsd libc [1]
(and hence apple libc [2]), they report EAI_NONAME error instead, so
adjust expectation.
Azat Khuzhin [Fri, 11 Jan 2019 18:52:11 +0000 (21:52 +0300)]
rpc: use *_new_with_arg() to match function prototype
In 755fbf16c ("Add void* arguments to request_new and reply_new
evrpc hooks") this new functions had been introduced, but newer used,
what for? So let's use them.
Azat Khuzhin [Fri, 7 Dec 2018 18:46:27 +0000 (21:46 +0300)]
http: fix connection retries when there more then one request for connection
We should not attemp to establishe the connection if there is retry
timer active, since otherwise there will be a bug.
Imagine next situation:
con = evhttp_connection_base_new()
evhttp_connection_set_retries(con, 2)
req = evhttp_request_new()
evhttp_make_request(con, req, ...)
# failed during connecting, and timer for 2 second scheduler (retry_ev)
Then another request scheduled for this evcon:
evhttp_make_request(con, req, ...)
# got request from server,
# and now it tries to read the response from the server
# (req.kind == EVHTTP_RESPONSE)
#
# but at this point retry_ev scheduled,
# and it schedules the connect again,
# and after the connect will succeeed, it will pick request with
# EVHTTP_RESPONSE for sending and this is completelly wrong and will
# fail in evhttp_make_header_response() since there is no
# "http_server" for this evcon
This was a long standing issue, that I came across few years ago
firstly, bad only now I had time to dig into it (but right now it was
pretty simple, by limiting amount of CPU for the process and using rr
for debug to go back and forth).
Azat Khuzhin [Thu, 22 Nov 2018 21:29:55 +0000 (00:29 +0300)]
cmake: do not build both (SHARED and STATIC) for MSVC/win32
MSVC does not support SHARED and STATIC libraries with the same name,
so let's just build SHARED libraries by default instead (yes we can add
prefix but let's stick with this).
The reason for this is that in windows shared libraries requires .lib
file too, but this is not static library it is imported library for
shared (doh...), for more info [1] and [2].
And when we build both static library can and will override shared
library imported part, let's take a look at event_extra.lib:
- before patch [3]:
$ less libevent-fail/lib/Debug/event_extra.lib | head
==> use library:contained_file to view a file in the archive
rw-rw-rw- 100666/100666 59568 Nov 21 23:55 2018 event_extra_static.dir/Debug/evrpc.obj
rw-rw-rw- 100666/100666 252219 Nov 21 23:55 2018 event_extra_static.dir/Debug/evdns.obj
rw-rw-rw- 100666/100666 203850 Nov 21 23:55 2018 event_extra_static.dir/Debug/http.obj
rw-rw-rw- 100666/100666 25907 Nov 21 23:55 2018 event_extra_static.dir/Debug/event_tagging.obj
- "after patch" [4] (not after but the same effect):
$ less libevent-ok/lib/Debug/event_extra.lib | head
==> use library:contained_file to view a file in the archive
--------- 0/0 509 Nov 21 23:38 2018 event_extra.dll
...
And there is no way to configure this (and indeed you need to do this
for MSVC for example), so let's introduce option for this --
EVENT__LIBRARY_TYPE.
Plus now we have INTERFACE libraries, that we can use internally in
libevent's cmake rules to avoid strict to _shared/_static variant of the
libraries to link with samples/tests (we prefer SHARED over STATIC for
linking).
Also bump minimal cmake required version to 3.1 by the following
reasons:
- 3.1 is required for RPATH configuration under APPLE
- 3.0 is required for add_library(INTERFACE) (did not found it in 2.8.x
documentation)
- remove extra conditions
(anyway 3.1 was release 4 years ago, so I guess that most of the systems
will have it)
Azat Khuzhin [Tue, 13 Nov 2018 21:20:20 +0000 (00:20 +0300)]
http: improve error path for bufferevent_{setfd,enable,disable}()
We have calls to the next functions but do not check return values,
though they can be invalid and it is better to show this somehow.
Also do bufferevent_setfd() first and only after it
bufferevent_enable()/bufferevent_disable() since:
a) it is more natural
b) it will avoid extra operations
c) it will not fail first bufferevent_enable() (this is the case for
buffbufferevent_async at least)
In this case we could add more information for issues like #709
Azat Khuzhin [Tue, 13 Nov 2018 19:47:43 +0000 (22:47 +0300)]
Merge branch 'iocp-fixes'
* iocp-fixes:
regress: test for HTTP/HTTPS with IOCP enabled
bev_async: trigger/run only deferred callbacks
bev_async: do not initialize timeouts multiple times
bev_async: set "ok" on setfd if fd>=0 (like we do during creation)
bev_async: ignore ERROR_INVALID_PARAMETER on .setfd for iocp
Azat Khuzhin [Tue, 13 Nov 2018 18:26:12 +0000 (21:26 +0300)]
Fix conceivable UAF of the bufferevent in evhttp_connection_free()
Although this is not a problem, since bufferevent uses finalizers and
will free itself only from the loop (well this is not a problem if you
do not play games with various event_base in different threads) it
generates questions, so rewrite it in more reliable way.
Azat Khuzhin [Tue, 13 Nov 2018 08:10:25 +0000 (11:10 +0300)]
Merge branch 'sample-http-server'
Some improvements for http-server sample:
- getopt
- persistent port via -p option
- IOCP for win32 via -I
- disable buffering
- enable debug logging via -v/EVENT_DEBUG_LOGGING_ALL
- cleanup (by signal and separate error path on errors)
* sample-http-server:
s/http-server: graceful cleanup
s/http-server: enable debug logging if EVENT_DEBUG_LOGGING_ALL env isset
s/http-server: turn off buffering (otherwise do output on win32)
s/http-server: add an option to use IOCP
s/http-server: add options (for persistent port)
Azat Khuzhin [Wed, 7 Nov 2018 21:36:07 +0000 (00:36 +0300)]
regress_ssl: fix ssl/bufferevent_wm_filter for non defered callbacks
Even after referenced patch there is still possible recursive callbacks
from evbuffer_drain(bev_input), i.e.:
wm_transfer() -> evbuffer_drain() -> wm_transfer()
inc(ctx->get)
But if we will increment ctx->get before drain that we will not add more
data to buffer.
Refs: 54c6fe3c ("regress_ssl: make ssl/bufferevent_wm_filter more fault-tolerance")
CI: https://ci.appveyor.com/project/nmathewson/libevent/build/job/f0rv299i71wnuxdq#L2546
(cherry picked from commit 1fc1c7ef1dabbbb358b99b893712bccc5761f82d)
Azat Khuzhin [Mon, 5 Nov 2018 14:04:47 +0000 (17:04 +0300)]
appveyor: cache build directory to reduce overall time (6x time faster)
various build checks (i.e. detecting headers/macroses/functions) takes
7 minutes (from 13 minutes in total) for cmake, which is too high.
By using cache we can reduce this to ~0.
And set APPVEYOR_SAVE_CACHE_ON_ERROR so that cmake checks will be
cached (anyway all sources will be built from scratch due to timestamp
updates while extracting from sources).
Azat Khuzhin [Mon, 5 Nov 2018 15:30:38 +0000 (18:30 +0300)]
cmake: set CMP0075 to NEW (for ws2_32.lib in win32)
Otherwise cmake complains:
Policy CMP0075 is not set: Include file check macros honor
CMAKE_REQUIRED_LIBRARIES. Run "cmake --help-policy CMP0075" for policy
details. Use the cmake_policy command to set the policy and suppress this
warning.
CMAKE_REQUIRED_LIBRARIES is set to:
ws2_32.lib
For compatibility with CMake 3.11 and below this check is ignoring it.
Azat Khuzhin [Mon, 5 Nov 2018 15:23:31 +0000 (18:23 +0300)]
cmake: set CMP0074 to NEW (for OPENSSL_ROOT in appveyor)
We have $env:OPENSSL_ROOT (env) equals to -DOPENSSL_ROOT (cmake
variable) anyway.
cmake complains:
Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
Run "cmake --help-policy CMP0074" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
Environment variable OpenSSL_ROOT is set to:
C:/OpenSSL-Win64/bin
For compatibility, CMake is ignoring the variable.
This warning is for project developers. Use -Wno-dev to suppress it.
Azat Khuzhin [Mon, 5 Nov 2018 19:25:15 +0000 (22:25 +0300)]
regress_ssl: make ssl/bufferevent_wm_filter more fault-tolerance
Due to inplace callbacks (i.e. no BEV_OPT_DEFER_CALLBACKS) we cannot be
sure that wm_transfer() will not be called recursively and indeed it
still happens sometimes, and the referenced patch increase amount of
this times, especially for linux/poll.
Fixes: 66304a23cf748714159c988e78f35401c5352827 ("Fix
ssl/bufferevent_wm_filter when bev does not reach watermark on break")
Azat Khuzhin [Mon, 5 Nov 2018 18:33:54 +0000 (21:33 +0300)]
regress_http: disable http/read_on_write_error under win32
EVHTTP_CON_READ_ON_WRITE_ERROR works only if an error already read from
the socket, but if we already got EPIPE on write we cannot read from the
socket anymore, and win32 does not guarantee that read will happens
before (although it happens from time to time).
In the referenced patch I just replaced callback with not expecting 417,
but like I already wrote, this is not always true (i.e. it is flacky).
Fixes: 3b581693ac1967f7f8d98491cb772a1b415eb4cd ("test/http:
read_on_write_error: fix it for win32")
Azat Khuzhin [Sun, 4 Nov 2018 18:41:20 +0000 (21:41 +0300)]
Merge branch 'ssl_bufferevent_wm_filter-fix'
* ssl_bufferevent_wm_filter-fix:
Fix ssl/bufferevent_wm_filter when bev does not reach watermark on break
regress_ssl: cover watermarks with deferred callbacks
regress_ssl: improve bufferevent_wm/bufferevent_wm_filter logging
Azat Khuzhin [Tue, 30 Oct 2018 22:22:30 +0000 (01:22 +0300)]
Merge branch 'event-ET-#636-v2'
* event-ET-#636-v2:
Preserve ET bit for backends with changelist
Epoll ET setting lost with multiple events for same fd
Cover ET with multiple events for same fd
Add ET flag into event_base_dump_events()
Azat Khuzhin [Sun, 28 Oct 2018 15:11:22 +0000 (18:11 +0300)]
Check existence of IPV6_V6ONLY in evutil_make_listen_socket_ipv6only() (mingw32)
MinGW 32-bit 5.3.0 does not defines it and our appveyour [1] build
reports this instantly:
evutil.c: In function 'evutil_make_listen_socket_ipv6only':
evutil.c:392:40: error: 'IPV6_V6ONLY' undeclared (first use in this function)
return setsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, (void*) &one,
Azat Khuzhin [Sun, 28 Oct 2018 13:46:24 +0000 (16:46 +0300)]
Convert evbuffer_strspn() (internal helper) to use size_t
As pointed by @yankeehacker in #590:
Signed to Unsigned Conversion Error - buffer.c:1623
Description: This assignment creates a type mismatch by populating an
unsigned variable with a signed value. The signed integer will be
implicitly cast to an unsigned integer, converting negative values into
positive ones. If an attacker can control the signed value, it may be
possible to trigger a buffer overflow if the value specifies the length
of a memory write.
Remediation: Do not rely on implicit casts between signed and unsigned
values because the result can take on an unexpected value and violate
weak assumptions made elsewhere in the program.
Azat Khuzhin [Sun, 28 Oct 2018 12:16:24 +0000 (15:16 +0300)]
buffer: add an assert for last_with_datap to suppress static analyzer
../buffer.c:2231:6: warning: Access to field 'flags' results in a dereference of a null pointer
if (CHAIN_SPACE_LEN(*firstchainp) == 0) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../buffer.c:130:30: note: expanded from macro 'CHAIN_SPACE_LEN'
#define CHAIN_SPACE_LEN(ch) ((ch)->flags & EVBUFFER_IMMUTABLE ? \
Azat Khuzhin [Sat, 27 Oct 2018 16:34:52 +0000 (19:34 +0300)]
regress_ssl: reset static variables on test setup/cleanup and eliminate leaks
One tricky bit is reply to the BIO_C_GET_FD command, since otherwise it
will try to close(0) and accepted bev in ssl/bufferevent_connect_sleep
will leak. Other seems more or less trivial.
This was done to make sure that for at least generic cases does not
leak (tricky cases was listed here nmathewson/Libevent#83).
Azat Khuzhin [Sat, 27 Oct 2018 15:35:08 +0000 (18:35 +0300)]
be_openssl: avoid leaking of SSL structure
From nmathewson/Libevent#83 by @fancycode:
There are a few code paths where the passed SSL object is not released in error cases, even if BEV_OPT_CLOSE_ON_FREE is passed as option while for others it is released. That way it's impossible for the caller to know it he has to free it on errors himself or not.
Line numbers are from "bufferevent_openssl.c" in 911abf3:
L1414 ("underlying == NULL" passed)
L1416 (bio could not be created)
L1446 (different fd passed)
L1325 (both underlying and fd passed)
L1328 (out-of-memory)
L1333 ("bufferevent_init_common_" failed)
In all error cases after the "bufferevent_ops_openssl" has been assigned, the option is evaluated on "bufferevent_free" (L1399) and the SSL object released (L1226).
Azat Khuzhin [Sat, 27 Oct 2018 11:58:30 +0000 (14:58 +0300)]
cmake: add various warning flags like autotools has
This is mostly to match autotools and reduce amount mixiing declarations
and code.
Added:
- -Wextra (the same as -W), -Wno-unused-parameter -Wstrict-aliasing
- -fno-strict-aliasing (gcc 2.9.5+)
- -Winit-self -Wmissing-field-initializers -Wdeclaration-after-statement (4.0+)
- -Waddress -Wno-unused-function -Wnormalized=id -Woverride-init (4.2+)
- -Wlogical-op (4.5+)
Removed:
- -Wformat (include in -Wall)
Plus use CMAKE_C_COMPILER_ID over CMAKE_COMPILER_IS_GNUCC, as
cmake-variables(7) suggesting, and add common GNUC/CLANG variables.
v2: drop checks for flags, since add_compiler_flags() will check if such
flags exists anyway (but just to note, gcc ignores non existing warning
flags by default).
Murat Demirten [Mon, 4 Jun 2018 13:43:34 +0000 (16:43 +0300)]
listener: ipv6only socket bind support
According to RFC3493 and most Linux distributions, default value is to
work in IPv4-mapped mode. If there is a requirement to bind same port
on same ip addresses but different handlers for both IPv4 and IPv6,
it is required to set IPV6_V6ONLY socket option to be sure that the
code works as expected without affected by bindv6only sysctl setting
in system.
Azat Khuzhin [Wed, 24 Oct 2018 21:50:50 +0000 (00:50 +0300)]
Merge branch 'evutil_found_ifaddr-dev'
* evutil_found_ifaddr-dev:
Cover evutil_v4addr_is_local_()/evutil_v6addr_is_local_()
Split evutil_found_ifaddr() into helpers (evutil_v{4,6}addr_is_local())
Use INADDR_ANY over 0 in evutil_found_ifaddr()
Replace EVUTIL_V4ADDR_IS_*() macroses with static inline functions
Filter link-local IPv4 addresses in evutil_found_ifaddr()
Azat Khuzhin [Mon, 22 Oct 2018 21:12:23 +0000 (00:12 +0300)]
Merge branch 'http-request-line-parsing'
* http-request-line-parsing:
Fix http https_basic/https_filter_basic under valgrind (increase timeout)
http: cover various non RFC3986 conformant URIs
http: allow non RFC3986 conformant during parsing request-line (http server)
http: do not try to parse request-line if we do not have enough bytes
http: allow trailing spaces (and only them) in request-line (like nginx)
http: cleanup of the request-line parsing
Azat Khuzhin [Sun, 21 Oct 2018 22:06:48 +0000 (01:06 +0300)]
Disable parallel jobs for the osx (due to CPU time deficit) in travis-ci
As you can see right now linux workers has zero failed tests, while osx
workers has 18 failed tests:
[bufferevent_connect_hostname_emfile FAILED]
[bufferevent_pair_release_lock FAILED]
[bufferevent_timeout FAILED]
[bufferevent_timeout_filter FAILED]
[bufferevent_timeout_pair FAILED]
[common_timeout FAILED]
[del_wait FAILED]
[immediatesignal FAILED]
[loopexit FAILED]
[loopexit_multiple FAILED]
[monotonic_res FAILED]
[no_events FAILED]
[persistent_active_timeout FAILED]
[persistent_timeout_jump FAILED]
[signal_switchbase FAILED]
[signal_while_processing FAILED]
[simpletimeout FAILED]
[usleep FAILED]
And this patch should remove from this list time related failures
(though maybe not all of them).
Azat Khuzhin [Sun, 21 Oct 2018 15:31:01 +0000 (18:31 +0300)]
Simplify bufferevent timeout tests to reduce CPU usage in between start/compare
Between start (setting "started_at") and comparing the time when
timeouts triggered with the start (test_timeval_diff_eq), there is too
much various things that can introduce extra delays and eventually could
fail the test on machine with shortage of CPU.
And this is exactly what happend on:
- travis-ci
- #262
Here is a simple reproducer that I came up with for this issue:
docker run --cpus=0.01 -e LD_LIBRARY_PATH=$PWD/lib -e PATH=/usr/bin:/bin:$PWD/bin -v $PWD:$PWD --rm -it debian:testing regress --no-fork --verbose bufferevent/bufferevent_timeout
Under limited CPU (see reproducer) the test almost always has problems
with that "write_timeout_at" exceed default timeval diff tolerance
(test_timeval_diff_eq() has 50 tolerance), i.e.:
FAIL ../test/regress_bufferevent.c:1040: assert(labs(timeval_msec_diff(((&started_at)), ((&res1.write_timeout_at))) - (100)) <= 50): 101 vs 50
But under some setup write timeout can even not triggered, and the
reason for this is that we write to the bufferevent 1024*1024 bytes, and
hence if evbuffer_write_iovec() will has some delay after writev() and
not send more then one vector at a time [1], it is pretty simple to
trigger, i.e.:
FAIL ../test/regress_bufferevent.c:1040: assert(labs(timeval_msec_diff(((&started_at)), ((&res1.write_timeout_at))) - (100)) <= 50): 1540155888478 vs 50
So this patch just send static small payload for all cases (plus a few
more asserts added).
The outcome of this patch is that all regression tests passed on
travis-ci for linux box [2]. While before it fails almost always [3].
Also reproducer with CPU limiting via docker also survive some
iterations (and strictly speaking it should has less CPU then travis-ci
workers I guess).
projects / libevent / log