Socket leakage on error

[hyperblast]

evhtp_bind_sockaddr() does not close created socket if bind() fails:
https://github.com/criticalstack/libevhtp/blob/develop/evhtp.c#L3668

if evhtp_accept_socket() fails for some reason, socked is leaked as well:
https://github.com/criticalstack/libevhtp/blob/develop/evhtp.c#L3673

[NathanFrench]

Thanks for the heads up, looking now.

[NathanFrench]

Hi again @hyperblast - I just pushed a quick patch here d13b72b

Seems to fix it; any suggestions?

[hyperblast]

Hi @NathanFrench, thanks for the fix.

I think it resolves the first issue, but second one still remains.

evhtp_accept_socket() gets called as the last statement of evhtp_bind_sockaddr().

Normally evhtp_accept_socket() gets ownership of fd because the evconnlistener_new() is called with LEV_OPT_CLOSE_ON_FREE option.

Assuming I understood libevent sources right evconnlistener_new() does not close socket when error occurs:

https://github.com/libevent/libevent/blob/a73fb2f443ebf9687ee6ca81a6401d1f3751683f/listener.c#L155

setsockopt() calls in the evhtp_accept_socket() might fail as well.

So to sum up, I think that evhtp_bind_sockaddr() should check result of call to evhtp_accept_socket() and close the socket on error to prevent leakage.

P.S. Thanks for having this project up and running. Keep it up!

[NathanFrench]

I'm not too familiar with Libevent's listener backend, but I think you're right. 18f7ec0 has been updated.

[hyperblast]

Hi Nathan,

I think the fix has possibly unintended side effect of changing evhtp_bind_sockaddr() logic to close socket on error.

It's a public function that could be called by library users directly (e.g. not via evhtp_bind_socket().

New behavior might be a good thing, but it's worth noting that this is a breaking change.

[NathanFrench]

We already make a determination on the ownership of a /connection/, which must be relinquished if the take_ownership function is called. But we don't do the same for file descriptors.

A user can pass their own file descriptor in, and we claim ownership, should we add a flag that specifically designates the owner of the file descriptor? Or maybe a better tree of perms:

base [api owned] -> server [api owned] -> connection [user/api owned] -> request -> [user/api owned]

[NathanFrench]

Ok, so this change technically should not break users if they used the function documentation :D. We weren't conforming to the documentation.

d0347dc should address most of these issues including an extra note.

[hyperblast]

Ok, so this change technically should not break users if they used the function documentation

It seems I've referenced wrong function in my previous comment, but you've already figured out what I've actually meant. Thank you.

A user can pass their own file descriptor in, and we claim ownership, should we add a flag that specifically designates the owner of the file descriptor?

Having externally owned socket might have a use-case. For example some kind of sort-restart. User wants to keep the listening socket, but recreate evhtp instance. However evhtp_accept_socket() calls listen() (via evconnlistener_new()) and setsockopt(). This might prevent socket reuse.
So to make this scenario work there should be a version of evconnlistener_new() that expects socket that is already in a listening state. AFAIK, there is no such function at the moment.

[NathanFrench]

I merged the changes with the proper checking and logic. (#8)

Not sure how to deal with clashes between libevent and evhtp other than sending in a patch. I think the ownership discussion here should be another ticket.

[NathanFrench]

Moved the rest to #9