Skip to content

http: add evhttp_connection_set_closecb to avoid g_requests hang #27909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Crypt-iQ wants to merge 1 commit into from
Closed

http: add evhttp_connection_set_closecb to avoid g_requests hang #27909

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/httpserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,21 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
{
WITH_LOCK(g_requests_mutex, g_requests.insert(req));
g_requests_cv.notify_all();

auto conn{evhttp_request_get_connection(req)};

// Close callback to clear active but running request. This is also
// called if the connection is closed after a successful request, but
// complete callback set below already cleared the state.
evhttp_connection_set_closecb(conn, [](evhttp_connection* conn, void* arg) {
auto req{static_cast<evhttp_request*>(arg)};
WITH_LOCK(g_requests_mutex, g_requests.erase(req));
g_requests_cv.notify_all();
}, req);
Copy link
Contributor

@stickies-v stickies-v Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the culprit of what you're observing. req becomes a dangling pointer as soon as a request is completed. In most cases (which is why we typically need > 1 number of iterations), this just means that g_requests.erase(req) removes 0 elements because it points to a non-existing request, but sometimes (and actually quite frequently as addresses seem to be reused quite aggressively) this will point to an unrelated request which has already been added into g_requests, erasing it unintentionally.

The underlying issue is that an evhttp_connection and an evhttp_request simply have different lifecycles, with connections living longer than requests. It seems that passing req as an argument to evhttp_connection_set_closecb is a fundamentally unsafe thing to do if we can't guarantee that req hasn't been completed by the time closecb is called (which in the happy path, we always expect it to be).

Copy link
Contributor Author

@Crypt-iQ Crypt-iQ Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

Copy link
Contributor

@vasild vasild Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this:

evhttp_connection_set_closecb(
    evhttp_request_get_connection(req),
    [](evhttp_connection* conn, void* arg) {
        auto req{static_cast<evhttp_request*>(arg)};
        // By the time the connection close callback is executed (this lambda)
        // the request we scheduled to be passed as an argument (arg) may
        // have been destroyed and a new, unrelated, one could have been
        // created at the same address. So arg may be a stale pointer or
        // a valid pointer, pointing to a new request object.
        LOCK(g_requests_mutex);
        if (g_requests.count(req) > 0 /* is not a stale pointer */
            && evhttp_request_get_connection(req) == conn /* is not pointing to a new request */) {

            g_requests.erase(req);
            g_requests_cv.notify_all();
        }
    },
    req);

Copy link
Contributor

@stickies-v stickies-v Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

            && evhttp_request_get_connection(req) == conn /* is not pointing to a new request */) {

I'm not sure this will be 100% robust when we have multiple requests in a connection, I think this can still lead to usage of stale pointers? But as we're closing the connection anyway, it may not be an issue.

Copy link
Contributor

@stickies-v stickies-v Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most sensible approach is to instead of track requests, keep a per-connection request counter. It's a pretty straightforward approach with a std::unordered_map<const evhttp_connection*, size_t>, but I've added a RequestTracker helper class wrapping around that to make that a bit more robust. What do you think?

stickies-v@953c691

Copy link
Contributor

@stickies-v stickies-v Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #28551 with this approach to try and get it in before v26.


// Clear state after successful request.
evhttp_request_set_on_complete_cb(req, [](struct evhttp_request* req, void*) {
auto n{WITH_LOCK(g_requests_mutex, return g_requests.erase(req))};
assert(n == 1);
WITH_LOCK(g_requests_mutex, g_requests.erase(req));
g_requests_cv.notify_all();
}, nullptr);
}
Expand Down