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

[Myk Taylor <myk002@xxxxxxxxx>]

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.