[Libevent-users] Re: Avoid potential SSL read spinlocks

2011-11-14 Thread Catalin Patulea 
Hi,

Why does bev_ssl require deferred callbacks?

Commit fc52dbac (see below) fixed an issue I was seeing where deferred
error_cb(EOF)s were confusing clients which had disabled EV_READs
(notably evhttp). If bev_ssl didn't require deferred cbs, the EOFs
would have stayed queued at the OpenSSL layer (in the same way that
EOFs normally stay queued in the kernel when bev_sock clients disable
EV_READ).

Thanks,
Catalin

diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c
index 86a8619..3843b31 100644
--- a/bufferevent_openssl.c
+++ b/bufferevent_openssl.c

-   while ((bev_ssl->bev.bev.enabled & EV_READ) &&
+   if ((bev_ssl->bev.bev.enabled & EV_READ) &&

r = do_read(bev_ssl, n_to_read);
-   if (r <= 0)
-   break;
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: Avoid potential SSL read spinlocks

2011-11-14 Thread Catalin Patulea 
On Mon, Nov 14, 2011 at 11:17 AM, Mark Ellzey  wrote:
> As far as I know, it doesn't. But I wrote this patch because if you are
> using a deferred bev with bulk incoming data, it never drops back into
> base_loop() until SSL_read() says there is nothing to read. This is due
> to the fact that your callback for new data is never called until
> libevent gets around to processing the deferred queue.
This is actually causing an assert in my case :( The key is to get
SSL_read to return both data (> 0) and EOF (= 0) within the same
iteration of that (former) while loop. Then the flow looks like this:

SSL_read() returns N bytes into an iov from evbuffer_reserve_space -
no run_read_cb yet
SSL_read() returns 0, which leads into conn_closed(), which queues a
error_cb(EOF)
Back in do_read, a read_cb is queued

In the event loop, the read_cb is delivered first, completing the HTTP
request. evhttp disables EV_READs and moves into state
EVCON_DISCONNECTED. ...and then the error_cb(EOF) is delivered,
confusing evhttp and causing assert(req != NULL) in
evhttp_connection_fail.

I've found it hard to point the finger at any particular piece of
code... It depends on whether the bufferevent interface defines an
ordering between EV_READ enable/disables and EOF/ERROR events.

In the case of bev_sock, once disable(EV_READ) has been called, the fd
is not checked for readability anymore, so read() doesn't have a
chance to report any EOF or errors. Effectively, we're leaving these
events queued in the kernel (which seems like a reasonable thing to do
- evhttp no longer cares about that fd).

Your patch indirectly does this - EOF/errors are now queued in OpenSSL
because SSL_read is only called once for every read_cb.

My only concern is that there may be cases where OpenSSL _needs_ us to
call SSL_read repeatedly to make any progress, despite the underlying
fd staying unreadable. Maybe during renegociation or application
payloads that span SSL records? The man page doesn't have anything to
that effect, but that doesn't mean the cases don't exist..

> This does not change any mechanics to openssl bufferevents without
> deferred enabled.
I agree that your patch doesn't affect non-deferred mechanics - but I
haven't been able to get evhttp to stack on top of bev_ssl without
deferred callbacks. It seems to send the request line, but gets stuck
in write_cb's before sending request headers. I haven't investigated
too much because it seems like it would only be a quick fix - sooner
or later someone may want to use deferred cbs with bev_ssl, for the
reasons described in the link you sent.

BTW, sample/le-proxy.c also enabled deferred cbs on its bev_ssl.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: Avoid potential SSL read spinlocks

2011-11-16 Thread Catalin Patulea 
On Tue, Nov 15, 2011 at 10:25 AM, Mark Ellzey  wrote:
> Also, would be nice to have some code that reproduces the issue you are
> having.
Ok, try the attached patch against 9ae6e595 (just before your fixes).
The use of an IP in the URL is incidental because of IPv6 issues on my
system - you can probably use www.google.com directly.

$ .libs/https-client https://74.125.224.80/

Response line: 302 Found
> 
302 Moved
302 Moved
The document has moved
http://www.google.com/";>here.


[err] ../libevent/http.c:704: Assertion req != NULL failed in
evhttp_connection_fail
Aborted
From 09e940ef9b71b612037e04b74f94879bfcbebd6a Mon Sep 17 00:00:00 2001
From: Catalin Patulea 
Date: Wed, 16 Nov 2011 11:22:04 -0800
Subject: [PATCH] Add sample/https-client.c, an example of stacking evhttp as a client on top of bufferevent_ssl.

This reproduces an assert(req != NULL) in evhttp_connection_fail for me.
---
 sample/Makefile.am|4 +
 sample/https-client.c |  205 +
 2 files changed, 209 insertions(+), 0 deletions(-)
 create mode 100644 sample/https-client.c

diff --git a/sample/Makefile.am b/sample/Makefile.am
index 61e72d6..2147b29 100644
--- a/sample/Makefile.am
+++ b/sample/Makefile.am
@@ -17,6 +17,10 @@ AM_CPPFLAGS += $(OPENSSL_INCS)
 noinst_PROGRAMS += le-proxy
 le_proxy_SOURCES = le-proxy.c
 le_proxy_LDADD = $(LDADD) ../libevent_openssl.la -lssl -lcrypto
+
+noinst_PROGRAMS += https-client
+https_client_SOURCES = https-client.c
+https_client_LDADD = $(LDADD) ../libevent_openssl.la -lssl -lcrypto
 endif
 
 verify:
diff --git a/sample/https-client.c b/sample/https-client.c
new file mode 100644
index 000..5013c68
--- /dev/null
+++ b/sample/https-client.c
@@ -0,0 +1,205 @@
+/*
+  This is an example of how to hook up evhttp with bufferevent_ssl
+
+  It just GETs an https URL given on the command-line and prints the response
+  body to stdout.
+
+  Actually, it also accepts plain http URLs to make it easy to compare http vs
+  https code paths.
+
+  Loosely based on le-proxy.c.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#ifdef WIN32
+#include 
+#include 
+#else
+#include 
+#include 
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+static struct event_base *base;
+
+static void
+http_request_done(struct evhttp_request *req, void *ctx)
+{
+	char buffer[256];
+	int nread;
+
+	if (req == NULL) {
+		fprintf(stderr, "some request failed - no idea which one though!\n");
+		return;
+	}
+
+	fprintf(stderr, "Response line: %d %s\n",
+		req->response_code, req->response_code_line);
+
+	while ((nread = evbuffer_remove(req->input_buffer, buffer, sizeof(buffer))) > 0) {
+		fwrite("> ", 2, 1, stdout);
+		fwrite(buffer, nread, 1, stdout);
+		fwrite("\n", 1, 1, stdout);
+	}
+}
+
+static void
+syntax(void)
+{
+	fputs("Syntax:\n", stderr);
+	fputs("   https-client \n", stderr);
+	fputs("Example:\n", stderr);
+	fputs("   https-client https://ipcheckit.com/\n";, stderr);
+
+	exit(1);
+}
+
+static void
+die(const char *msg)
+{
+	fputs(msg, stderr);
+	exit(1);
+}
+
+int
+main(int argc, char **argv)
+{
+	int r;
+
+	struct evhttp_uri *http_uri;
+	const char *url, *scheme, *host, *path, *query;
+	char uri[256];
+	int port;
+
+	SSL_CTX *ssl_ctx;
+	SSL *ssl;
+	struct bufferevent *bev;
+	struct evhttp_connection *evcon;
+	struct evhttp_request *req;
+
+	if (argc != 2)
+		syntax();
+
+	url = argv[1];
+	http_uri = evhttp_uri_parse(url);
+	if (http_uri == NULL) {
+		die("malformed url");
+	}
+
+	scheme = evhttp_uri_get_scheme(http_uri);
+	if (scheme == NULL || (strcasecmp(scheme, "https") != 0 &&
+	   strcasecmp(scheme, "http") != 0)) {
+		die("url must be http or https");
+	}
+
+	host = evhttp_uri_get_host(http_uri);
+	if (host == NULL) {
+		die("url must have a host");
+	}
+
+	port = evhttp_uri_get_port(http_uri);
+	if (port == -1) {
+		port = (strcasecmp(scheme, "http") == 0) ? 80 : 443;
+	}
+
+	path = evhttp_uri_get_path(http_uri);
+	if (path == NULL) {
+		path = "/";
+	}
+
+	query = evhttp_uri_get_query(http_uri);
+	if (query == NULL) {
+		snprintf(uri, sizeof(uri) - 1, "%s", path);
+	} else {
+		snprintf(uri, sizeof(uri) - 1, "%s?%s", path, query);
+	}
+	uri[sizeof(uri) - 1] = '\0';
+
+	// Initialize OpenSSL
+	SSL_library_init();
+	ERR_load_crypto_strings();
+	SSL_load_error_strings();
+	OpenSSL_add_all_algorithms();
+	r = RAND_poll();
+	if (r == 0) {
+		fprintf(stderr, "RAND_poll() failed.\n");
+		return 1;
+	}
+	ssl_ctx = SSL_CTX_new(SSLv23_method());
+
+	// Create event base
+	base = event_base_new();
+	if (!base) {
+		perror("event_base_new()");
+		return 1;
+	}
+
+	// Create OpenSSL bufferevent and stack evhttp on to

Re: [Libevent-users] Re: Avoid potential SSL read spinlocks

2011-11-16 Thread Catalin Patulea 
A few things I forgot to mention:

Re: bev_ssl needing deferred or not, without OPT_DEFER_CALLBACKS,
https-client does not work for me. I haven't investigated.

Re: SSL_read loops. Consider the following scenario:

- Peer sends two (small) SSL records in a row (perhaps in the same TCP segment)
- fd becomes readable, libevent delivers read_cb to bev_ssl
- bev_ssl calls SSL_read only once - does this read process only the
first record or both?

If it only processes the first record, then we should call SSL_read
again. But the fd is no longer readable, so libevent won't dispatch
read_cb. The second second record is queued somewhere above BIO but
below SSL_read, and we get stuck.

But just to emphasize - this entirely depends on (AFAICT undocumented)
internal behaviour of SSL_read. If this were read(2) then the fd would
remain readable from poll() so we would get a second read_cb, emptying
the kernel queue.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: Avoid potential SSL read spinlocks

2011-11-16 Thread Catalin Patulea 
On Wed, Nov 16, 2011 at 11:52 AM, Catalin Patulea  wrote:
> - Peer sends two (small) SSL records in a row (perhaps in the same TCP 
> segment)
> - fd becomes readable, libevent delivers read_cb to bev_ssl
> - bev_ssl calls SSL_read only once - does this read process only the
> first record or both?
Ah, no, appdata records get coalesced when read through SSL_read.
Verified this using Wireshark and logging SSL_read calls.

Now I'm starting to wonder about whether close notifies would get
delivered (assuming the client leaves EV_READ enabled)..
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

[Libevent-users] bev_ssl: Be more specific in event callbacks

2011-11-21 Thread Catalin Patulea 
evhttp is very strict when checking the event flags while reading the
response body:

evhttp_error_cb(struct bufferevent *bufev, short what, void *arg)
{

case EVCON_READING_BODY:
if (!req->chunked && req->ntoread < 0
&& what == (BEV_EVENT_READING|BEV_EVENT_EOF)) {
/* EOF on read can be benign */
evhttp_connection_done(evcon);
return;
}
break;

bev_ssl currently only reports EVENT_EOF when the connection is
closed, so it confuses evhttp. This patch makes bev_ssl additionally
set one of BEV_EVENT_{READING|WRITING}, as appropriate, when the
connection is closed.
From f7d21c69ac8b0c57fab6ba4c7afe2250c646c35e Mon Sep 17 00:00:00 2001
From: Catalin Patulea 
Date: Mon, 21 Nov 2011 19:24:50 -0500
Subject: [PATCH 1/2] bev_ssl: Be more specific in event callbacks. evhttp in particular gets confused without at least one of BEV_EVENT_{READING|WRITING}.

---
 bufferevent_openssl.c |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c
index 89497fd..3ca906b 100644
--- a/bufferevent_openssl.c
+++ b/bufferevent_openssl.c
@@ -483,7 +483,7 @@ clear_wbor(struct bufferevent_openssl *bev_ssl)
 }
 
 static void
-conn_closed(struct bufferevent_openssl *bev_ssl, int errcode, int ret)
+conn_closed(struct bufferevent_openssl *bev_ssl, int when, int errcode, int ret)
 {
 	int event = BEV_EVENT_ERROR;
 	int dirty_shutdown = 0;
@@ -529,6 +529,8 @@ conn_closed(struct bufferevent_openssl *bev_ssl, int errcode, int ret)
 	stop_reading(bev_ssl);
 	stop_writing(bev_ssl);
 
+	/* when is BEV_EVENT_{READING|WRITING} */
+	event = when | event;
 	_bufferevent_run_eventcb(&bev_ssl->bev.bev, event);
 }
 
@@ -604,7 +606,7 @@ do_read(struct bufferevent_openssl *bev_ssl, int n_to_read)
 		return -1;
 break;
 			default:
-conn_closed(bev_ssl, err, r);
+conn_closed(bev_ssl, BEV_EVENT_READING, err, r);
 break;
 			}
 			blocked = 1;
@@ -682,7 +684,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
 bev_ssl->last_write = space[i].iov_len;
 break;
 			default:
-conn_closed(bev_ssl, err, r);
+conn_closed(bev_ssl, BEV_EVENT_WRITING, err, r);
 bev_ssl->last_write = -1;
 break;
 			}
@@ -979,7 +981,7 @@ do_handshake(struct bufferevent_openssl *bev_ssl)
 			}
 			return 0;
 		default:
-			conn_closed(bev_ssl, err, r);
+			conn_closed(bev_ssl, BEV_EVENT_READING, err, r);
 			return -1;
 		}
 	}
-- 
1.7.3.1

[Libevent-users] [PATCH] bev_ssl: Allow users to set allow_dirty_shutdown

2011-11-21 Thread Catalin Patulea 
By default, allow_dirty_shutdown is 0, reporting
TCP-close-before-SSL-close as BEV_EVENT_ERROR.

But many https servers out there do dirty shutdowns, so clients need
to be able to set this flag.

This patch simply adds a getter/setter for the flag. Default behaviour
of bev_ssl does not change.
From 9632ddb90393fcb4a8b537644d932e8eedd08c1c Mon Sep 17 00:00:00 2001
From: Catalin Patulea 
Date: Mon, 21 Nov 2011 19:57:19 -0500
Subject: [PATCH 2/2] Allow users to set allow_dirty_shutdown

---
 bufferevent_openssl.c|   23 ++-
 include/event2/bufferevent_ssl.h |   16 
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c
index 3ca906b..6f13159 100644
--- a/bufferevent_openssl.c
+++ b/bufferevent_openssl.c
@@ -313,7 +313,7 @@ struct bufferevent_openssl {
 	unsigned read_blocked_on_write : 1;
 	/* When we next get data, we should say "write" instead of "read". */
 	unsigned write_blocked_on_read : 1;
-	/* XXX */
+	/* Treat TCP close before SSL close on SSL >= v3 as clean EOF. */
 	unsigned allow_dirty_shutdown : 1;
 	/*  */
 	unsigned fd_is_set : 1;
@@ -1389,6 +1389,27 @@ bufferevent_openssl_socket_new(struct event_base *base,
 		base, NULL, fd, ssl, state, options);
 }
 
+int bufferevent_openssl_get_allow_dirty_shutdown(struct bufferevent *bev)
+{
+	int allow_dirty_shutdown = 0;
+	struct bufferevent_openssl *bev_ssl;
+	BEV_LOCK(bev);
+	bev_ssl = upcast(bev);
+	allow_dirty_shutdown = bev_ssl->allow_dirty_shutdown;
+	BEV_UNLOCK(bev);
+	return allow_dirty_shutdown;
+}
+
+void bufferevent_openssl_set_allow_dirty_shutdown(struct bufferevent *bev,
+int allow_dirty_shutdown)
+{
+	struct bufferevent_openssl *bev_ssl;
+	BEV_LOCK(bev);
+	bev_ssl = upcast(bev);
+	bev_ssl->allow_dirty_shutdown = allow_dirty_shutdown;
+	BEV_UNLOCK(bev);
+}
+
 unsigned long
 bufferevent_get_openssl_error(struct bufferevent *bev)
 {
diff --git a/include/event2/bufferevent_ssl.h b/include/event2/bufferevent_ssl.h
index bf6009a..30bf2d3 100644
--- a/include/event2/bufferevent_ssl.h
+++ b/include/event2/bufferevent_ssl.h
@@ -88,6 +88,22 @@ bufferevent_openssl_socket_new(struct event_base *base,
 enum bufferevent_ssl_state state,
 int options);
 
+/** Control whether to report dirty SSL shutdowns.
+
+If the peer closes the TCP connection before closing the SSL channel, the
+protocol is SSL >= v3, and allow_dirty_shutdown=0 (default), you will receive
+BEV_EVENT_ERROR.
+
+If instead allow_dirty_shutdown=1, you will receive BEV_EVENT_EOF.
+
+On the other hand, if the protocol is < SSLv3, you will always receive
+BEV_EVENT_EOF.
+*/
+
+int bufferevent_openssl_get_allow_dirty_shutdown(struct bufferevent *bev);
+void bufferevent_openssl_set_allow_dirty_shutdown(struct bufferevent *bev,
+int allow_dirty_shutdown);
+
 /** Return the underlying openssl SSL * object for an SSL bufferevent. */
 struct ssl_st *
 bufferevent_openssl_get_ssl(struct bufferevent *bufev);
-- 
1.7.3.1

Re: [Libevent-users] detection of TCP disconnection

2011-11-22 Thread Catalin Patulea 
On Tue, Nov 22, 2011 at 7:18 PM,   wrote:
> However if i a do hard reset(power off) of the machine running the client,
> it does not seem to work and I believe I am not getting a callback
> function for the event on the server.
I think the fundamental problem is distinguishing between 'hard reset'
and 'network temporarily down'. In both cases, the clients sees it as
silence from the server for some (arbitrarily long) period of time.

Most apps just send application-level pings at some interval and
handle unanswered pings in some manner that is appropriate to the
application.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] [PATCH] bev_ssl: Allow users to set allow_dirty_shutdown

2011-11-24 Thread Catalin Patulea 
On Thu, Nov 24, 2011 at 12:34 PM, Nick Mathewson  wrote:
> I tidied the documentation a little in a44cd2b0205df; and fixed a
> couple of robustness issues in f3b89dec9eac2cf: you might want to
> check those out to make sure I didn't mess it up for you.
Still works fine here, thanks!
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] PATCH: bufferevent_openssl consider_read doesn't drain underlying BE in all cases

2011-12-19 Thread Catalin Patulea 
On Mon, Dec 19, 2011 at 5:34 PM, Haseeb Abdul Qadir
 wrote:
> 
Thanks for resending this on the mailing list.

> SSL_pending() ends up returning 0 in some cases even when the underlying BE's 
> input buffer has data left to consume
This is exactly the condition I was afraid of back when 2aa036f was
being discussed. I was never able to come up with a concrete scenario,
but it seemed conceivable. Note that I was using an OpenSSL bev
layered directly on top of BIO_socket, not looping back over an
underlying bev.

OTOH, if SSL_pending can return 0 with bytes queued in the evbuffer,
then these bytes could just as well be queued in the kernel, when
layering over BIO_socket. So it might be possible to reproduce the
condition with bev_ssl-over-BIO_socket too.

> I've created the patch below and it seems to fix the issue on my end.
> https://github.com/haseebq/libevent/commit/c58d9d8d437d122a8b67d42a6126d81699ec6914
I'm a bit confused by the patch - n_to_read is a number of cleartext
bytes to be read from SSL, right? You're setting it from
evbuffer_get_length, which is a number of encrypted bytes on the
underlying bev (below SSL).

> Ps - I haven't tried to reproduce this bug in a stand alone project yet.
Would you be able to provide a repro, or at least some more in-depth
description of the conditions that prompted the issue to surface?
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: evhttp: persistent connections over ssl with chunked transfer encoding bug?

2011-12-22 Thread Catalin Patulea 
On Thu, Dec 22, 2011 at 4:55 PM, Hochhaus, Andrew
 wrote:
> You can also reproduce the behavior in a web browser by running the
> server (main.cc), browsing to https://127.0.0.1:8889/ and then
> refreshing a few times (until one of the TCP connections is reused).
I can reproduce this too against master. (I tweaked the code just a
little to read certs from disk) Here is my output from http.py:

$ ./http.py
host=www.google.com ssl=False chunked=None:  success
host=www.google.com ssl=True chunked=True:  success
host=127.0.0.1 ssl=False chunked=None:  success
host=127.0.0.1 ssl=True chunked=None:  success
host=127.0.0.1 ssl=False chunked=True:  success
host=127.0.0.1 ssl=True chunked=None:  failure

My code is at: https://github.com/cpatulea/Libevent/tree/ssltimeout

I'll be digging into this to try to figure out what's going on.

Catalin
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: evhttp: persistent connections over ssl with chunked transfer encoding bug?

2011-12-22 Thread Catalin Patulea 
On Thu, Dec 22, 2011 at 5:12 PM, Catalin Patulea  wrote:
> I'll be digging into this to try to figure out what's going on.
Seems like bev_ssl flushes data to SSL_write and notifies back more
eagerly than bev_sock, which is confusing evhttp_send_reply_end.

evhttp_send_reply_end tries to write the last chunk and get notified
(send_done) when it's been written:

evhttp_send_reply_end(struct evhttp_request *req) {
<...>
if (req->chunked) {
evbuffer_add(output, "0\r\n\r\n", 5);
evhttp_write_buffer(req->evcon, evhttp_send_done, NULL);

evhttp_write_buffer is basically:
/* Set call back */
evcon->cb = cb;
evcon->cb_arg = arg;

bufferevent_enable(evcon->bufev, EV_WRITE);

The problem is that bev_ssl flushes the write and fires the write
callback from within evbuffer_add, which is too early. When
write_buffer does enable(EV_WRITE), no data is being flushed anymore,
so the event never gets fired.

bev_sock just asks to be notified for fd writability, and returns.
write_buffer gets the chance to prepare a callback. Then the bev_sock
writecb actually flushes the evbuffer to write(2).

If it is deemed acceptable for a bev to flush and cb from
evbuffer_add, then maybe bev_ssl should also fire a write_cb upon
enable(EV_WRITE) when everything has already been flushed.

Otherwise (and it seems most of evhttp is built on this assumption),
bev_ssl should never flush anything unless it's being called from the
event loop.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

[Libevent-users] [PATCH] Force strict validation of HTTP version in response

2012-01-16 Thread Catalin Patulea 
Hi,

I ran into this while evhttp was trying to fetch Shoutcast streams.
The server responds with 'ICY 200 OK', which libevent intends to
reject. But the sscanf return value isn't checked properly and the
behaviour is undefined (in practice, sometimes accepts the response,
sometimes rejects it).

For small patches like this, should I just paste the patch in the
message body? It would make it easier to glance at the patch, though
there's a possibility Gmail might mangle the text.

Catalin
From 429903e6385823d780631734a959ddae4401069c Mon Sep 17 00:00:00 2001
From: Catalin Patulea 
Date: Tue, 10 Jan 2012 18:33:58 -0500
Subject: [PATCH] Force strict validation of HTTP version in response.

This sometimes accepted invalid versions like 'ICY' (n = 0, major = undefined, sometimes > 1).
---
 http.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 1fccc2c..67b985a 100644
--- a/http.c
+++ b/http.c
@@ -1468,7 +1468,7 @@ evhttp_parse_http_version(const char *version, struct evhttp_request *req)
 	int major, minor;
 	char ch;
 	int n = sscanf(version, "HTTP/%d.%d%c", &major, &minor, &ch);
-	if (n > 2 || major > 1) {
+	if (n != 2 || major > 1) {
 		event_debug(("%s: bad version %s on message %p from %s",
 			__func__, version, req, req->remote_host));
 		return (-1);
-- 
1.7.3.1

[Libevent-users] evdns timeout behaviour

2012-07-06 Thread Catalin Patulea 
Hi,

I'm interested in what happens when a freshly created evdns with more than
one nameserver encounters a timeout on the first request.

It seems evdns distinguishes between retransmits and reissues. Retransmits
are triggered by timeouts and always point to the same ns, chosen at
first-transmit time. Reissues, on the other hand, are triggered by bad
responses from nameservers, like NOTIMPL or REFUSED, and do cause a new ns
to be picked in a round-robin fashion.

The net effect is that on a fresh evdns with 2 nameservers, a single
evdns_resolve is tried only against one nameserver. A second evdns_resolve
is needed to talk to the second ns.

This is different from glibc (2.7), where a new nameserver appears to be
chosen before each retransmit. I'm not sure whether glibc makes any
distinction between retransmit and reissue.

Can anyone comment on these semantics? What was the rationale? Is there any
way to get evdns to behave like glibc without significant code changes? (I
don't see anything obvious in evdns_set_option.)

Catalin

Re: [Libevent-users] evdns timeout behaviour

2012-07-09 Thread Catalin Patulea 
On Sat, Jul 7, 2012 at 4:54 PM, Adrian Chadd  wrote:
> Eg, what does the bind "libc" implementation of name lookups do? (ie,
> what user applications can do, not what BIND itself does.)
Any idea what Debian package has this implementation? The only
user-accessible resolver I can find in libbind-dev is lwres. lwres
rotates for each rexmit (grouped to show bursts):

14:08:44.281278 IP 172.29.33.50.55508 > 2.3.4.5.53: 10299+% [1au] A?
www.google.ca. (42)
14:08:44.281362 IP 172.29.33.50.24086 > 2.3.4.5.53: 39714+ [1au] NS? . (28)

14:08:46.281595 IP 172.29.33.50.29057 > 1.2.3.4.53: 23708+% [1au] A?
www.google.ca. (42)
14:08:46.281645 IP 172.29.33.50.10276 > 1.2.3.4.53: 65136+ [1au] NS? . (28)

14:08:48.324396 IP 172.29.33.50.62960 > 2.3.4.5.53: 32037+% [1au] A?
www.google.ca. (42)

14:08:50.324731 IP 172.29.33.50.22628 > 1.2.3.4.53: 53006+% [1au] A?
www.google.ca. (42)
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] evdns timeout behaviour

2012-07-09 Thread Catalin Patulea 
On Sat, Jul 7, 2012 at 4:54 PM, Adrian Chadd  wrote:
> .. what do other operating systems do?
BTW, glibc has a "rotate" option that load-balances across the list of
nameservers - but this option only applies when starting a new lookup
[1]. In the timeout case, a new nameserver is always tried [2].

My understanding is that multiple nameservers in glibc were originally
intended for failover only, and client-side load-balancing (the rotate
option) was added later as a feature.

[1] 
http://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/res_send.c;h=0a28cd784b99d45eb70150f3ac50b75b14203976;hb=HEAD#l460
[2] 
http://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/res_send.c;h=0a28cd784b99d45eb70150f3ac50b75b14203976;hb=HEAD#l563
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

[Libevent-users] [PATCH] http: on connection reset, detach the closed fd from the bufferevent

2012-12-19 Thread Catalin Patulea 
From: Catalin Patulea 

I'm curious what people think of a patch like this.. I've been seeing fds 
getting reused and bufferevents adding/removing events on the underlying evmap 
when they shouldn't be touching that fd anymore.

This patch seems to fix it but I haven't fully considered the reprecussions of 
doing this.

Any first impressions on this?

Thanks,
Catalin
---
 http.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index 22a46a9..8d871fd 100644
--- a/http.c
+++ b/http.c
@@ -1240,6 +1240,7 @@ evhttp_connection_reset_(struct evhttp_connection *evcon)
tmp = bufferevent_get_input(evcon->bufev);
evbuffer_drain(tmp, evbuffer_get_length(tmp));
 
+   bufferevent_setfd(evcon->bufev, -1);
evcon->state = EVCON_DISCONNECTED;
 }
 
-- 
1.7.7.3

***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

[Libevent-users] Re: [PATCH] http: on connection reset, detach the closed fd from the bufferevent

2013-02-19 Thread Catalin Patulea 
[-cc: my personal email, added by mistake]

Ping. We've been seeing good results with this.

On Wed, Dec 19, 2012 at 11:05 AM, Catalin Patulea  wrote:
> From: Catalin Patulea 
>
> I'm curious what people think of a patch like this.. I've been seeing fds 
> getting reused and bufferevents adding/removing events on the underlying 
> evmap when they shouldn't be touching that fd anymore.
>
> This patch seems to fix it but I haven't fully considered the reprecussions 
> of doing this.
>
> Any first impressions on this?
>
> Thanks,
> Catalin
> ---
>  http.c |1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 22a46a9..8d871fd 100644
> --- a/http.c
> +++ b/http.c
> @@ -1240,6 +1240,7 @@ evhttp_connection_reset_(struct evhttp_connection 
> *evcon)
> tmp = bufferevent_get_input(evcon->bufev);
> evbuffer_drain(tmp, evbuffer_get_length(tmp));
>
> +   bufferevent_setfd(evcon->bufev, -1);
> evcon->state = EVCON_DISCONNECTED;
>  }
>
> --
> 1.7.7.3
>
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: [PATCH] http: on connection reset, detach the closed fd from the bufferevent

2013-02-19 Thread Catalin Patulea 
On Tue, Feb 19, 2013 at 11:49 AM, Nick Mathewson  wrote:
> Could you be more specific about the good results, and how to
> reproduce them?  Or how to reproduce/observe the undesired behavior?
> It would also be good to have an explanation of how the patch is
> supposed to work.
I had a lot of trouble reproducing this bug. It is exacerbated by high
requests parallelism, where there is a high chance of having fds
numbers get reused by the kernel and the evmap falls out of sync with
the kernel's idea of which fds are and aren't used.

I think the trigger is a connection that times out. Then
evhttp_connection_reset_ closes the fd (evutil_closesocket) but the
bufferevent still thinks it owns the fd and the fd is still present in
the underlying evmap/epoll implementation.

Then the kernel hands the same fd to a new bufferevent and that
bufferevent starts getting events for the old one. This would cause
assertions in the epoll layer like "Epoll ADD(X) on fd Y failed",
Valgrind invalid reads or invalid writes and occasionally crashes.

I'm sorry I can't be more specific than this. The way I tackled this
was by adding a ton of cross-layer assertions like ("fd returned from
accept is not present in evmap") or "fd passed to evutil_closesocket
must have no remaining event interests attached to it" and gradually
tracing back to who was violating these assumptions.

We saw a 7-fold reduction in crash rate with this patch.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

[Libevent-users] [PATCH] Add sample/https-client.c, an example of stacking evhttp as a client on top of bufferevent_ssl.

2013-02-19 Thread Catalin Patulea 

Signed-off-by: Catalin Patulea 
---
 .gitignore|   1 +
 sample/https-client.c | 207 ++
 sample/include.am |   5 ++
 3 files changed, 213 insertions(+)
 create mode 100644 sample/https-client.c

diff --git a/.gitignore b/.gitignore
index 057ec08..dad74e7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@ libevent_openssl.pc
 /sample/hello-world
 /sample/http-server
 /sample/le-proxy
+/sample/https-client
 /sample/signal-test
 /sample/time-test
 
diff --git a/sample/https-client.c b/sample/https-client.c
new file mode 100644
index 000..1ea6e27
--- /dev/null
+++ b/sample/https-client.c
@@ -0,0 +1,207 @@
+/*
+  This is an example of how to hook up evhttp with bufferevent_ssl
+
+  It just GETs an https URL given on the command-line and prints the response
+  body to stdout.
+
+  Actually, it also accepts plain http URLs to make it easy to compare http vs
+  https code paths.
+
+  Loosely based on le-proxy.c.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#ifdef WIN32
+#include 
+#include 
+#else
+#include 
+#include 
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+static struct event_base *base;
+
+static void
+http_request_done(struct evhttp_request *req, void *ctx)
+{
+   char buffer[256];
+   int nread;
+
+   if (req == NULL) {
+   fprintf(stderr, "some request failed - no idea which one 
though!\n");
+   return;
+   }
+
+   fprintf(stderr, "Response line: %d %s\n",
+   req->response_code, req->response_code_line);
+
+   while ((nread = evbuffer_remove(req->input_buffer, buffer, 
sizeof(buffer)))
+  > 0) {
+   fwrite("> ", 2, 1, stdout);
+   fwrite(buffer, nread, 1, stdout);
+   fwrite("\n", 1, 1, stdout);
+   }
+}
+
+static void
+syntax(void)
+{
+   fputs("Syntax:\n", stderr);
+   fputs("   https-client \n", stderr);
+   fputs("Example:\n", stderr);
+   fputs("   https-client https://ip.appspot.com/\n";, stderr);
+
+   exit(1);
+}
+
+static void
+die(const char *msg)
+{
+   fputs(msg, stderr);
+   exit(1);
+}
+
+int
+main(int argc, char **argv)
+{
+   int r;
+
+   struct evhttp_uri *http_uri;
+   const char *url, *scheme, *host, *path, *query;
+   char uri[256];
+   int port;
+
+   SSL_CTX *ssl_ctx;
+   SSL *ssl;
+   struct bufferevent *bev;
+   struct evhttp_connection *evcon;
+   struct evhttp_request *req;
+
+   if (argc != 2)
+   syntax();
+
+   url = argv[1];
+   http_uri = evhttp_uri_parse(url);
+   if (http_uri == NULL) {
+   die("malformed url");
+   }
+
+   scheme = evhttp_uri_get_scheme(http_uri);
+   if (scheme == NULL || (strcasecmp(scheme, "https") != 0 &&
+  strcasecmp(scheme, "http") != 0)) {
+   die("url must be http or https");
+   }
+
+   host = evhttp_uri_get_host(http_uri);
+   if (host == NULL) {
+   die("url must have a host");
+   }
+
+   port = evhttp_uri_get_port(http_uri);
+   if (port == -1) {
+   port = (strcasecmp(scheme, "http") == 0) ? 80 : 443;
+   }
+
+   path = evhttp_uri_get_path(http_uri);
+   if (path == NULL) {
+   path = "/";
+   }
+
+   query = evhttp_uri_get_query(http_uri);
+   if (query == NULL) {
+   snprintf(uri, sizeof(uri) - 1, "%s", path);
+   } else {
+   snprintf(uri, sizeof(uri) - 1, "%s?%s", path, query);
+   }
+   uri[sizeof(uri) - 1] = '\0';
+
+   // Initialize OpenSSL
+   SSL_library_init();
+   ERR_load_crypto_strings();
+   SSL_load_error_strings();
+   OpenSSL_add_all_algorithms();
+   r = RAND_poll();
+   if (r == 0) {
+   fprintf(stderr, "RAND_poll() failed.\n");
+   return 1;
+   }
+   ssl_ctx = SSL_CTX_new(SSLv23_method());
+
+   // Create event base
+   base = event_base_new();
+   if (!base) {
+   perror("event_base_new()");
+   return 1;
+   }
+
+   // Create OpenSSL bufferevent and stack evhttp on top of it
+   ssl = SSL_new(ssl_ctx);
+   if (ssl == NULL) {
+   fprintf(stderr, "SSL_new() failed\n");
+   return 1;
+   }
+
+   if (strcasecmp(scheme, "http") == 0) {
+   bev = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE);
+   } else {
+   bev = bufferevent_openssl_socket_new(base, -1, ssl,
+

Re: [Libevent-users] Re: [PATCH] http: on connection reset, detach the closed fd from the bufferevent

2013-02-19 Thread Catalin Patulea 
On Tue, Feb 19, 2013 at 4:17 PM, Nick Mathewson  wrote:
> But the patch isn't quite optimal for that -- we should do the
> bufferevent_setfd() before we close the socket, not after.  That way
> there's never a non-deleted event that refers to the fd.
The idea with doing the setfd there was in the case where evcon->fd ==
-1 but the bufferevent has an fd.

Why is there a copy of the fd in evcon? Why not use
bufferevent_getfd(evcon->bufev)?

> How does the attached patch look to you? Does it work as well/any better?
I'll see about canarying it and will reply once it's been running for a while.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] Re: [PATCH] http: on connection reset, detach the closed fd from the bufferevent

2013-02-19 Thread Catalin Patulea 
I don't know. I did the debugging on this about 3 weeks ago and my
cache is very cold at the moment.

Shouldn't bufferevent_disable_hard_(bufev, EV_READ|EV_WRITE) have
cleared any fd out of the evmap and epoll?

Looking at be_socket_disable:

if (event & EV_READ) {
  if (event_del(&bufev->ev_read) == -1)
return -1;
}
/* Don't actually disable the write if we are trying to connect. */
if ((event & EV_WRITE) && ! bufev_p->connecting) {
  if (event_del(&bufev->ev_write) == -1)
return -1;
}

In particular the '&& !connecting' part. Seems related to the
occurrence of these on fds that time out.

On Tue, Feb 19, 2013 at 4:30 PM, Catalin Patulea  wrote:
> On Tue, Feb 19, 2013 at 4:17 PM, Nick Mathewson  wrote:
>> But the patch isn't quite optimal for that -- we should do the
>> bufferevent_setfd() before we close the socket, not after.  That way
>> there's never a non-deleted event that refers to the fd.
> The idea with doing the setfd there was in the case where evcon->fd ==
> -1 but the bufferevent has an fd.
>
> Why is there a copy of the fd in evcon? Why not use
> bufferevent_getfd(evcon->bufev)?
>
>> How does the attached patch look to you? Does it work as well/any better?
> I'll see about canarying it and will reply once it's been running for a while.
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] [PATCH] Add sample/https-client.c, an example of stacking evhttp as a client on top of bufferevent_ssl.

2013-02-19 Thread Catalin Patulea 
On Tue, Feb 19, 2013 at 3:05 PM, Nick Mathewson  wrote:
>* It could sure use comments!
Can you be more specific? This all feels like a lot of boilerplate to
me. Parse the URL, initialize OpenSSL, create some bufferevents. I'm
not sure what more I can say that a reader of bufferevent.h,
bufferevent_ssl.sh and SSL_new(3) etc. doesn't already know.

>* This is dangerous code; it doesn't do any certificate validation
> so far as I can see, and as such gets zero protection from
> man-in-the-middle attacks.  People who don't know how to use TLS will
> be copying our examples here, so we need to make sure to get the
> security right.
SSL_CTX_set_verify(SSL_VERIFY_PEER, NULL); sound about right to you?

I'm trying to figure out whether OpenSSL distributes a set of CA certs
and initializes the path or whether I need to do this myself - any
idea?
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.

Re: [Libevent-users] [PATCH] Add sample/https-client.c, an example of stacking evhttp as a client on top of bufferevent_ssl.

2013-02-20 Thread Catalin Patulea 
On Tue, Feb 19, 2013 at 9:40 PM, Jardel Weyrich  wrote:
> 2) Call SSL_CTX_load_verify_locations passing the path of the CA
> certificates installed by the aforementioned package - generally
> /etc/ssl/certs/ca-certificates.crt
Nick, does this seem like a reasonable solution?

SSL_CTX_load_verify_locations(ssl_ctx,
"/etc/ssl/certs/ca-certificates.crt", NULL);

Anything more than this feels, to me, outside the scope of a libevent sample.

In a related vein, is it possible to get OpenSSL to immediately dump
errors to stderr? The only API I can find (ERR_*) let you inspect
errors after the fact (ERR_get_error, ERR_print_error, etc.) but I
would prefer not to clutter the sample with those calls unless
necessary?
***
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-usersin the body.