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.

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

Fix arguments of dispatched functions cannot be actually moved #7873

Open
wants to merge 2 commits into
base: RC_2_0
Choose a base branch
from
Open

Fix arguments of dispatched functions cannot be actually moved #7873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b72434
Fix use-after-move in test_storage
glassez Feb 16, 2025
cc0b207
Fix arguments of dispatched functions cannot be actually moved
glassez Feb 15, 2025
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
25 changes: 12 additions & 13 deletions src/session_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Copyright (c) 2014-2018, Steven Siloti
Copyright (c) 2015-2018, Alden Torres
Copyright (c) 2015-2022, Arvid Norberg
Copyright (c) 2025, Vladimir Golovnev (glassez)
All rights reserved.

Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -92,12 +93,12 @@
{
std::shared_ptr<session_impl> s = m_impl.lock();
if (!s) aux::throw_ex<system_error>(errors::invalid_session_handle);
dispatch(s->get_context(), [=]() mutable
dispatch(s->get_context(), std::bind([s, f](auto&&... args) mutable

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable args is never read.

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable args is not used.

Check notice

Code scanning / CodeQL

Declaration hides parameter Note

Local variable 'args' hides a
parameter of the same name
Loading
.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be beneficial to move function f into the lambda too if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be beneficial to move function f into the lambda too if possible.

Generally, yes. But in this case, it is intended only for pointer to member function, so moving it will not add any advantages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. So it is a pointer rather than a function object...

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

Suggested change
dispatch(s->get_context(), std::bind([s, f](auto&&... args) mutable
dispatch(s->get_context(), std::bind([s, f](Args&&... args) mutable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not:

It is incorrect.

{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
(s.get()->*f)(std::forward<Args>(a)...);
(s.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (system_error const& e) {
s->alerts().emplace_alert<session_error_alert>(e.code(), e.what());
Expand All @@ -107,7 +108,7 @@
s->alerts().emplace_alert<session_error_alert>(error_code(), "unknown error");
}
#endif
});
}, std::forward<Args>(a)...));
}

template<typename Fun, typename... Args>
Expand All @@ -122,12 +123,12 @@
bool done = false;

std::exception_ptr ex;
dispatch(s->get_context(), [=, &done, &ex]() mutable
dispatch(s->get_context(), std::bind([s, f, &done, &ex](auto&&... args) mutable
{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
(s.get()->*f)(std::forward<Args>(a)...);
(s.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (...) {
ex = std::current_exception();
Expand All @@ -136,7 +137,7 @@
std::unique_lock<std::mutex> l(s->mut);
done = true;
s->cond.notify_all();
});
}, std::forward<Args>(a)...));

aux::torrent_wait(done, *s);
if (ex) std::rethrow_exception(ex);
Expand All @@ -154,12 +155,12 @@
bool done = false;
Ret r;
std::exception_ptr ex;
dispatch(s->get_context(), [=, &r, &done, &ex]() mutable
dispatch(s->get_context(), std::bind([s, f, &r, &done, &ex](auto&&... args) mutable

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable args is never read.

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable args is not used.

Check notice

Code scanning / CodeQL

Declaration hides parameter Note

Local variable 'args' hides a
parameter of the same name
Loading
.
{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
r = (s.get()->*f)(std::forward<Args>(a)...);
r = (s.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (...) {
ex = std::current_exception();
Expand All @@ -168,7 +169,7 @@
std::unique_lock<std::mutex> l(s->mut);
done = true;
s->cond.notify_all();
});
}, std::forward<Args>(a)...));

aux::torrent_wait(done, *s);
if (ex) std::rethrow_exception(ex);
Expand Down Expand Up @@ -426,8 +427,7 @@
handle_backwards_compatible_resume_data(params);
#endif
error_code ec;
auto ecr = std::ref(ec);
torrent_handle r = sync_call_ret<torrent_handle>(&session_impl::add_torrent, std::move(params), ecr);
torrent_handle r = sync_call_ret<torrent_handle>(&session_impl::add_torrent, std::move(params), std::ref(ec));
if (ec) aux::throw_ex<system_error>(ec);
return r;
}
Expand Down Expand Up @@ -460,8 +460,7 @@
#if TORRENT_ABI_VERSION == 1
handle_backwards_compatible_resume_data(params);
#endif
auto ecr = std::ref(ec);
return sync_call_ret<torrent_handle>(&session_impl::add_torrent, std::move(params), ecr);
return sync_call_ret<torrent_handle>(&session_impl::add_torrent, std::move(params), std::ref(ec));
}

torrent_handle session_handle::add_torrent(add_torrent_params const& params, error_code& ec)
Expand Down
29 changes: 15 additions & 14 deletions src/torrent_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
Copyright (c) 2018, Steven Siloti
Copyright (c) 2019, Andrei Kurushin
Copyright (c) 2019, ghbplayer
Copyright (c) 2025, Vladimir Golovnev (glassez)
All rights reserved.

Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -122,12 +123,12 @@
std::shared_ptr<torrent> t = m_torrent.lock();
if (!t) aux::throw_ex<system_error>(errors::invalid_torrent_handle);
auto& ses = static_cast<session_impl&>(t->session());
dispatch(ses.get_context(), [=,&ses] ()
dispatch(ses.get_context(), std::bind([t, f, &ses](auto&&... args) mutable

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable args is never read.

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable args is not used.

Check notice

Code scanning / CodeQL

Declaration hides parameter Note

Local variable 'args' hides a
parameter of the same name
Loading
.
{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
(t.get()->*f)(std::move(a)...);
(t.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (system_error const& e) {
ses.alerts().emplace_alert<torrent_error_alert>(torrent_handle(t)
Expand All @@ -140,7 +141,7 @@
, error_code(), "unknown error");
}
#endif
} );
}, std::forward<Args>(a)...));
}

template<typename Fun, typename... Args>
Expand All @@ -154,12 +155,12 @@
bool done = false;

std::exception_ptr ex;
dispatch(ses.get_context(), [=,&done,&ses,&ex] ()
dispatch(ses.get_context(), std::bind([t, f, &done, &ses, &ex](auto&&... args) mutable
{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
(t.get()->*f)(std::move(a)...);
(t.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (...) {
ex = std::current_exception();
Expand All @@ -168,7 +169,7 @@
std::unique_lock<std::mutex> l(ses.mut);
done = true;
ses.cond.notify_all();
} );
}, std::forward<Args>(a)...));

aux::torrent_wait(done, ses);
if (ex) std::rethrow_exception(ex);
Expand All @@ -190,12 +191,12 @@
bool done = false;

std::exception_ptr ex;
dispatch(ses.get_context(), [=,&r,&done,&ses,&ex] ()
dispatch(ses.get_context(), std::bind([t, f, &r, &done, &ses, &ex](auto&&... args) mutable

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable args is never read.

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable args is not used.

Check notice

Code scanning / CodeQL

Declaration hides parameter Note

Local variable 'args' hides a
parameter of the same name
Loading
.
{
#ifndef BOOST_NO_EXCEPTIONS
try {
#endif
r = (t.get()->*f)(std::move(a)...);
r = (t.get()->*f)(std::forward<Args>(args)...);
#ifndef BOOST_NO_EXCEPTIONS
} catch (...) {
ex = std::current_exception();
Expand All @@ -204,7 +205,7 @@
std::unique_lock<std::mutex> l(ses.mut);
done = true;
ses.cond.notify_all();
} );
}, std::forward<Args>(a)...));

aux::torrent_wait(done, ses);

Expand Down Expand Up @@ -511,8 +512,8 @@

void torrent_handle::piece_availability(std::vector<int>& avail) const
{
auto availr = std::ref(static_cast<aux::vector<int, piece_index_t>&>(avail));
sync_call(&torrent::piece_availability, availr);
auto& arg = static_cast<aux::vector<int, piece_index_t>&>(avail);
sync_call(&torrent::piece_availability, std::ref(arg));
}

void torrent_handle::piece_priority(piece_index_t index, download_priority_t priority) const
Expand Down Expand Up @@ -641,7 +642,8 @@
#if !TORRENT_NO_FPU
void torrent_handle::file_progress(std::vector<float>& progress) const
{
sync_call(&torrent::file_progress_float, std::ref(static_cast<aux::vector<float, file_index_t>&>(progress)));
auto& arg = static_cast<aux::vector<float, file_index_t>&>(progress);
sync_call(&torrent::file_progress_float, std::ref(arg));
}
#endif

Expand Down Expand Up @@ -796,8 +798,7 @@
entry torrent_handle::write_resume_data() const
{
add_torrent_params params;
auto retr = std::ref(params);
sync_call(&torrent::write_resume_data, resume_data_flags_t{}, retr);
sync_call(&torrent::write_resume_data, resume_data_flags_t{}, std::ref(params));
return libtorrent::write_resume_data(params);
}

Expand Down
9 changes: 5 additions & 4 deletions test/test_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Copyright (c) 2005, 2007-2010, 2012-2022, Arvid Norberg
Copyright (c) 2016, 2018, Alden Torres
Copyright (c) 2016, Andrei Kurushin
Copyright (c) 2016-2018, Steven Siloti
Copyright (c) 2016, Vladimir Golovnev
Copyright (c) 2016, 2025, Vladimir Golovnev (glassez)
Copyright (c) 2018, d-komarov
All rights reserved.

Expand Down Expand Up @@ -928,11 +928,12 @@ void test_fastresume(bool const test_deprecated)
p.storage_mode = storage_mode_sparse;
error_code ignore;
torrent_handle h = ses.add_torrent(std::move(p), ignore);
TEST_CHECK(exists(combine_path(p.save_path, "temporary")));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is noteworthy that this "use-after-move" bug also went unnoticed, as p was not actually moved.

if (!exists(combine_path(p.save_path, "temporary")))

torrent_status s = h.status();
TEST_CHECK(exists(combine_path(s.save_path, "temporary")));
if (!exists(combine_path(s.save_path, "temporary")))
return;

torrent_status s;
for (int i = 0; i < 50; ++i)
{
print_alerts(ses, "ses");
Expand Down
Loading