[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [Libevent-users] [PATCH] client HTTPS with evhttp



On 04/10/12 17:03, Nick Mathewson wrote:
https://sourceforge.net/tracker/?func=detail&aid=3516647&group_id=50884&atid=461324

   * Does this really belong on 2.0 or not? From your description of
the changes, it doesn't seem like a bugfix to me; it sounds much more
like a new feature.  I'm open to argument, though. (Mark, why did you
think this was a bugfix?)
It was actually me that originally filed in the Bugs tracker. I guess whether it's a new feature or a bugfix could depend on the merge target. For 2.1 I would think it's a bugfix since 2.1 provides the evhttp_connection_base_bufferevent_new() call which was behaving poorly when used with openssl bufferevents. For 2.0 I'd think it would be a new feature, since the patch provides the capability for HTTPS client connections.

   * Is it possible to write a test for this?
regress_http.c would have to be extended with an https server, but absolutely.

   * Probably, bev_factory_cb should have a name starting with evhttp_,
since it's evhttp-specific thing.  It should probably take the
evhttp_connection and maybe the event_base too?
the name change is simple enough. I'll change that in my next drop. the event base is set for the returned bufferevent inside http.c (just as it is for the existing evhttp_connection_base_bufferevent_new()). Why would the callback need the evhttp_connection pointer in addition to it's void *?

I'm also hoping to hear what Mark Ellzey thinks of the interface;
since I see his libevhtp as the eventual successor to evhttp, I'd like
his opinion on new evhttp APIs.
When is it planned to switch over to libevhtp? Is it worth hashing this out and testing it if it's just going to get replaced?

On that note, my patch has one known (to me) bug that I could use some advice on. The bug can happen without my patch too, given the right network conditions. The problem is in evhttp_connection_reset() where the socket is shutdown() and close()d. The bufferevent still has a reference to the closed file descriptor, and will attempt to manipulate it when bufferevent_free() is eventually called from evhttp_connection_free(). This could be really bad if some other bufferevent has reused the file descriptor for a new socket.

freeing the bufferevent in evhttp_connection_reset() and setting evcon->bufev = NULL is one option, but then a /lot/ of code would need to be changed throughout http.c to check for the possibility that evcon->bufev == NULL.

It would be nice if we could just reset the fd in the bufferevent to -1 and make it not attempt to use the fd anymore, but that doesn't seem to work: bufferevent_setfd(evcon->bufev, -1) calls be_openssl_ctrl() which unconditionally sets bev_ssl->fd_is_set = 1.

Sorry for the long email, but do you have any advice?

Thanks,
Myk
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.