net: fix use-after-free in tests #18376

vasild commented at 12:05 PM on March 18, 2020: member

In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda function to execute later, capturing the local variable consensusParams by reference.

Presumably this was considered safe because consensusParams is a reference itself to a global variable which is not supposed to change, but it can in tests.

Fixes #18372

net: fix use-after-free in tests

In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.

Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.

Fixes https://github.com/bitcoin/bitcoin/issues/18372

7d8e1dec3b

fanquake added the label P2P on Mar 18, 2020

fanquake requested review from MarcoFalke on Mar 18, 2020

vasild commented at 12:58 PM on March 18, 2020: member

Configure and run as: ./configure --with-sanitizers=thread ./src/test/test_bitcoin --run_test="txvalidationcache_tests/checkinputs_test"

I confirm the following:

Latest master @ ce87d5613 shows the use-after-free bug
This PR fixes the bug
Reverting fadafb83c on top of the latest master git show fadafb83c |git apply -R -3 (+ trivial conflict resolution) fixes the bug
The failure seems to be deterministic

So, this was introduced in #18289. I have no clue why travis was green for it.

-    scheduler.scheduleEvery(std::bind(&PeerLogicValidation::CheckForStaleTipAndEvictPeers, this, consensusParams), EXTRA_PEER_CHECK_INTERVAL * 1000);
+    scheduler.scheduleEvery([&] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});

Indeed std::bind() copies the arguments whereas the introduced lambda function passes them by reference and will brick if the argument is destroyed before the function is called.

in src/net_processing.cpp:1130 in 7d8e1dec3b

1126 | @@ -1127,7 +1127,7 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CS
1127 |      // combine them in one function and schedule at the quicker (peer-eviction)
1128 |      // timer.
1129 |      static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
1130 | -    scheduler.scheduleEvery([&] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
1131 | +    scheduler.scheduleEvery([this, consensusParams] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});

MarcoFalke commented at 1:35 PM on March 18, 2020:

I think this can be made = to just mirror what std::bind did. (this is a pointer, so it shouldn't matter whether it is copied or referenced, and for consensusParams we want it to be copied).

    scheduler.scheduleEvery([=] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});

MarcoFalke commented at 1:51 PM on March 18, 2020:

The = is also what is recommended in the "Tutorial" (written by me, so count that against me):

https://github.com/bitcoin/bitcoin/blob/3a8d25064e700ff2e69600cc1ede597751283a85/src/scheduler.h#L28

vasild commented at 3:16 PM on March 18, 2020:

Doh! I was about to apply this suggestion, but then realized that The implicit capture of *this when the capture default is = is deprecated. (since C++20) :-/ Indeed:

        auto m() {
            return [=](){
                std::cout << "lambda: this=" << this << std::endl;
            };
        }

Produces:

clang++10 -std=c++20 -Wdeprecated t.cc -o t
t.cc:96:49: warning: implicit capture of 'this' with a capture default of '=' is deprecated
      [-Wdeprecated-this-capture]
                std::cout << "lambda: this=" << this << std::endl;
                                                ^
t.cc:95:21: note: add an explicit capture of 'this' to capture '*this' by reference
            return [=](){
                    ^
                     , this

Better not add code that would have to be fixed later, so I would rather keep it as is.

What do you think?

PS this is always captured by reference and I think all of the following do the same wrt this: [=], [this], [&], std::bind

MarcoFalke commented at 4:39 PM on March 18, 2020:

In that case I'd prefer to use [=, this], but this might not compile either on our current C++ target? According to the page you linked:

    [=, this] {};   // until C++20: Error: this when = is the default
                    // since C++20: OK, same as [=]

So, given that we won't be switching to C++20 any time soon, I'd still prefer the [=]. Then it can be changed, if and when we switch to C++20?

Just a nit, though. ACK from me either way.

MarcoFalke approved

MarcoFalke commented at 1:36 PM on March 18, 2020: member

ACK 7d8e1dec3b26074df1533f715871f79c956cc224

In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda function to execute later, capturing the local variable consensusParams by reference.

Presumably this was considered safe because consensusParams is a reference itself to a global variable which is not supposed to change, but it can in tests.

Correct, it is a reference to a global, which I assumed to not change after initialization. This does not hold for unit test, for example the txvalidation one:

https://github.com/bitcoin/bitcoin/blob/ce87d5613a5537b68cf23e7bc25c1e3770704ff9/src/test/util/setup_common.cpp#L160-L166

practicalswift commented at 4:59 PM on March 18, 2020: contributor

Concept ACK -- premature optimisation is the root of many lifetime issues :)

This is a great talk about recurring C++ bugs at Facebook: this segment on lifetime issues due to premature optimization might have some relevance here :)

MarcoFalke commented at 5:53 PM on March 18, 2020: member

@practicalswift I don't think it is helpful to call this premature optimization. This was not an optimization attempt. GetConsensus returns a const reference because this global is assumed to be initialized once and then never changed. It has been that way for years, and I presume well known by myself and all reviewers of the pull. See also bd006110fb51f1fc0cbbeef3ed6eaae66b296d8c. And logically, it wouldn't make sense for a running Bitcoin Core instance to change the consensus rules "on the fly" in a running process.

I think it was reasonable to keep it a reference and pass it along in the scheduler. And I assume the four people who reviewed my pull request agreed with me.

That some unit tests spin up a PeerLogicValidation and then change the consensus parameters while running is documented as a temporary hack in the test code. And the oversight was that not everyone had this temporary hack in mind while writing/reviewing the code.

sipa commented at 8:11 PM on March 18, 2020: member

ACK 7d8e1dec3b26074df1533f715871f79c956cc224

practicalswift commented at 8:35 PM on March 18, 2020: contributor

ACK 7d8e1dec3b26074df1533f715871f79c956cc224

[…] premature optimisation is the root of many lifetime issues :)

I don't think it is helpful to call this premature optimization.

Point taken :)

MarcoFalke merged this on Mar 18, 2020

MarcoFalke closed this on Mar 18, 2020

fanquake referenced this in commit 92735e45ba on Aug 26, 2020

sidhujag referenced this in commit 914f824016 on Aug 26, 2020

DrahtBot locked this on Feb 15, 2022