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

Method called on moved-from object will cause undeterminate behaviour #569

Open
goddardl opened this issue Dec 1, 2022 · 0 comments
Open
Labels
bug Something isn't working

Comments

@goddardl
Copy link

goddardl commented Dec 1, 2022

Hello,
There are several issues in Trie::find (crow/routing.h) where various code paths could invoke undefined behaviour by calling a method on a std::vector that has previously been moved.

One such example is highlighted below (at line 895 of crow/routing.h):
// These next two lines move the data pointed to by "blueprints" to the local variable "found_BP".
895: auto ret = find(req_url, child, pos + fragment.size(), params, blueprints); // <-- Moves the data pointed to by "blueprints" into the returned tuple
896: update_found(ret); // <-- Moves the same data to the "found_BP" variable.
897: if (!blueprints->empty()) blueprints->pop_back(); // <-- ERROR: We use the contents of "blueprints" after they've previously been moved.

It would appear that the developer was trying to optimize the code and avoid copying the std::vector "blueprints" where possible but has instead introduced several bugs where the moved object is being used. There are several ways to resolve the issue but it is unclear what the developer's intent was and so I cannot provide a patch without more information.

The output of clang-tidy highlights on of the errors in detail below:

[build] ./_deps/crow-src/include/crow/routing.h:897:51: error: Method called on moved-from object 'MT' of type 'std::vector' [clang-analyzer-cplusplus.Move,-warnings-as-errors]
[build] if (!blueprints->empty()) blueprints->pop_back();
[build] ^
[build] /home/rnd/dev/xport-in-source/dev/modules/UnitTest/Xport/src/Test/CrowTest.cpp:22:7: note: Calling 'Crow::handle'
[build] app.handle(req, res);
[build] ^~~~~~~~~~~~~~~~~~~~
[build] ./deps/crow-src/include/crow/app.h:74:13: note: Calling 'Router::handle'
[build] router
.handle(req, res);
[build] ^~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:1448:17: note: Assuming field 'method' is < InternalMethodCount
[build] if (req.method >= HTTPMethod::InternalMethodCount)
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:1448:13: note: Taking false branch
[build] if (req.method >= HTTPMethod::InternalMethodCount)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:1450:22: note: Assuming field 'method' is not equal to Head
[build] else if (req.method == HTTPMethod::Head)
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:1450:18: note: Taking false branch
[build] else if (req.method == HTTPMethod::Head)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:1455:22: note: Assuming field 'method' is not equal to Options
[build] else if (req.method == HTTPMethod::Options)
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:1455:18: note: Taking false branch
[build] else if (req.method == HTTPMethod::Options)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:1505:26: note: Calling 'Trie::find'
[build] auto found = trie.find(req.url);
[build] ^~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:750:13: note: Taking true branch
[build] if (params == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:754:13: note: Taking true branch
[build] if (blueprints == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:762:13: note: Taking true branch
[build] if (node == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:775:17: note: Assuming the condition is false
[build] if (pos == req_url.size())
[build] ^~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:775:13: note: Taking false branch
[build] if (pos == req_url.size())
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:785:21: note: Assuming field 'param' is equal to MAX
[build] if (child->param != ParamType::MAX)
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:785:17: note: Taking false branch
[build] if (child->param != ParamType::MAX)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:891:25: note: Assuming the condition is true
[build] if (req_url.compare(pos, fragment.size(), fragment) == 0)
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:891:21: note: Taking true branch
[build] if (req_url.compare(pos, fragment.size(), fragment) == 0)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:894:29: note: Assuming 'INVALID_BP_ID' is equal to field 'blueprint_index'
[build] if (child->blueprint_index != INVALID_BP_ID) blueprints->push_back(child->blueprint_index);
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:894:25: note: Taking false branch
[build] if (child->blueprint_index != INVALID_BP_ID) blueprints->push_back(child->blueprint_index);
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:895:36: note: Calling 'Trie::find'
[build] auto ret = find(req_url, child, pos + fragment.size(), params, blueprints);
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:750:13: note: Taking false branch
[build] if (params == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:754:13: note: Taking false branch
[build] if (blueprints == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:762:13: note: Taking false branch
[build] if (node == nullptr)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:775:17: note: Assuming the condition is false
[build] if (pos == req_url.size())
[build] ^~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:775:13: note: Taking false branch
[build] if (pos == req_url.size())
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:902:18: note: 'found_fragment' is false
[build] if (!found_fragment)
[build] ^~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:902:13: note: Taking true branch
[build] if (!found_fragment)
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:903:17: note: Object 'MT' of type 'std::vector' is left in a valid but unspecified state after move
[build] found_BP = std::move(*blueprints);
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:895:36: note: Returning from 'Trie::find'
[build] auto ret = find(req_url, child, pos + fragment.size(), params, blueprints);
[build] ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:897:29: note: Assuming the condition is true
[build] if (!blueprints->empty()) blueprints->pop_back();
[build] ^~~~~~~~~~~~~~~~~~~~
[build] ./_deps/crow-src/include/crow/routing.h:897:25: note: Taking true branch
[build] if (!blueprints->empty()) blueprints->pop_back();
[build] ^
[build] ./_deps/crow-src/include/crow/routing.h:897:51: note: Method called on moved-from object 'MT' of type 'std::vector'
[build] if (!blueprints->empty()) blueprints->pop_back();
[build] ^~~~~~~~~~~~~~~~~~~~~~
[build] 177712 warnings generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants