Comments (10)
Thanks for the heads up, looking now.
from libevhtp.
Hi again @hyperblast - I just pushed a quick patch here d13b72b
Seems to fix it; any suggestions?
from libevhtp.
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!
from libevhtp.
I'm not too familiar with Libevent's listener backend, but I think you're right. 18f7ec0 has been updated.
from libevhtp.
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.
from libevhtp.
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]
from libevhtp.
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.
from libevhtp.
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.
from libevhtp.
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.
from libevhtp.
Moved the rest to #9
from libevhtp.
Related Issues (20)
- Q: how to peek at incoming http requests in pipelined mode
- Q: Is it safe to operate the same request between different threads? HOT 1
- use evbuffer_add_reference send json data have a problem HOT 2
- how to build libevhtp on windows
- Log.c is not cross-platform file.
- Empty pkgconfig libdir
- Inconsistency between EVHTP_BUILD_SHARED and BUILD_SHARED_LIBS CMake options HOT 2
- How to use multithreading HOT 5
- multiple nc request in keepalive fails to increase keepalive count HOT 1
- Does libevhtp support https? If support, what is the tps of http and https with keepalive or non-keepalive?
- Migrating from ev_http: How to get the socket for evhtp_accept_socket()? HOT 1
- Add example server with "1 thread per client connection"
- evhtp.c##htp__create_reply_() missed "buf = request->rc_scratch" after "request->rc_scratch = evbuffer_new()"?
- log-format $status is request status, not response status, why ?
- crash while invoking evhtp_connection_new() HOT 1
- Major slowdown using ssl and add_buffer(req->buffer_out,...) HOT 1
- Invalid content length value results in empty response
- info: libevhtp relies on onigposix.h, which is disabled in onig>=6.9.5_rev1 HOT 1
- Seperate http request thread and worker thread
- Is libevhtp support Server-send Events?
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from libevhtp.