Skip to content
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.

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

Use references explicitly, and avoid dangling references captured by … #924

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulharris
Copy link
Contributor

…complete_request_handler_

After an exception is thrown, I often get SEGFAULTs. Not always.
So I checked with valgrind, and it reliably picks up a problem:
(trimmed for brevity)

==196458== Thread 2:
==196458== Use of uninitialised value of size 8
==196458==    at 0x212A0C: std::vector<int, std::allocator<int> >::size() const (stl_vector.h:993)
==196458==    by 0x2A5697: crow::detail::middleware_call_criteria_dynamic<true>::middleware_call_criteria_dynamic(std::vector<int, std::allocator<int> > const&) (middleware.h:312)
==196458==    by 0x1D1FB1: crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}::operator()() const (routing.h:1750)
==196458==    by 0x1D1F74: void std::__invoke_impl<void, crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}&>(std::__invoke_other, crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}&) (invoke.h:61)
==196458==    by 0x1D1F34: std::enable_if<is_invocable_r_v<void, crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}&>, void>::type std::__invoke_r<void, crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}&>(crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}&) (invoke.h:111)
==196458==    by 0x1D1DCC: std::_Function_handler<void (), crow::Router::handle_rule<crow::Crow<TEMPLATE> >(crow::BaseRule*, crow::request&, crow::response&, crow::routing_params const&)::{lambda()#2}>::_M_invoke(std::_Any_data const&) (std_function.h:290)
==196458==    by 0x20FEF4: std::function<void ()>::operator()() const (std_function.h:591)
==196458==    by 0x20FD42: crow::response::end() (http_response.h:250)
==196458==    by 0x1D0B76: void crow::Router::handle<crow::Crow<TEMPLATE> >(crow::request&, crow::response&, crow::routing_handle_result) (routing.h:1722)
==196458==    by 0x1CCAEF: crow::Crow<TEMPLATE>::handle(crow::request&, crow::response&, std::unique_ptr<crow::routing_handle_result, std::default_delete<crow::routing_handle_result> >&) (app.h:238)
==196458==    by 0x1CC4D5: crow::Connection<TEMPLATE>::handle() (http_connection.h:202)
==196458==    by 0x1CBC2B: crow::HTTPParser<crow::Connection<TEMPLATE> >::process_message() (parser.h:162)

There are many more rows of this in different contexts.

So the problem is handle_rule(BaseRule* rule) brings rule in as a plain pointer parameter.
Then, this line creates a lambda that captures the reference to that pointer parameter.
https://github.com/CrowCpp/Crow/blob/25ca2f54e19e7c266d55fb3485d315d639d3f3f0/include/crow/routing.h#L1761C1-L1761C113
That reference becomes dangling after handle_rule() completes.

The complete_request_handler is later called and the reference to the pointer is now dangling.

There are three ways to solve this.
One is to change handle_rule(BaseRule* rule) to accept a reference to the pointer, ie handle_rule(BaseRule*& rule)
That means it would capture the pointer from way back on this line:

auto& rule = rules[rule_index];

The location of that pointer doesn't change, it is an entry in the rules vector which isn't supposed to change after the app starts ... right? Actually I'm not sure if that is a valid assumption.

Anyway, that approach ... I find that confusing.
You could make it look more consistent with the rest of the code by changing it to an auto&
handle_rule(auto& rule)
then all the parameters are references to something... but it is still hard to reason about lifetimes.

Instead, capture by copy. It's a pointer, there is no reason to capture by reference.
so this line:
https://github.com/CrowCpp/Crow/blob/25ca2f54e19e7c266d55fb3485d315d639d3f3f0/include/crow/routing.h#L1761C1-L1761C113
becomes:
res.complete_request_handler_ = [rule, &ctx, &container, &req, &res, glob_completion_handler] {

But that means the code becomes inconsistent. Some are captured by ref, some by pointer. Hard to know what it should be.

The other way is to use a reference to the rule.
This makes the assumption of lifetimes explicit, and makes the code consistent. And we can capture by reference as before.
ie, handle_rule(BaseRule& rule)
and the call:

auto& rule = rules[rule_index];

becomes:
BaseRule& rule = rules[rule_index];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant