test: Plug memory leaks and stack-use-after-scope #12477

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1802-qaMemLeaks changing 3 files +13 −9

MarcoFalke commented at 5:20 AM on February 18, 2018: member

No description provided.
MarcoFalke added the label Tests on Feb 18, 2018

in src/bench/coin_selection.cpp:56 in fac85e5f94 outdated

  52 | +
  53 | +        // Empty wallet.
  54 | +        for (COutput& output : vCoins) {
  55 | +            delete output.tx;
  56 | +        }
  57 | +        vCoins.clear();

promag commented at 2:43 PM on February 19, 2018:

Nit, std::vector<COutput> vCoins could be declared inside while() to avoid explicit clear.

in src/test/checkqueue_tests.cpp:409 in fac85e5f94 outdated

 405 | @@ -406,11 +406,11 @@ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
 406 |          boost::thread_group tg;
 407 |          std::mutex m;
 408 |          std::condition_variable cv;
 409 | +        bool has_lock{false};

promag commented at 2:46 PM on February 19, 2018:

Nice catch.

promag commented at 2:46 PM on February 19, 2018: member

utACK fac85e5.
practicalswift commented at 3:12 PM on February 19, 2018: contributor

@MarcoFalke Very nice! Thanks! Identified using valgrind?

utACK fac85e5f94c71dd8adbd6921d1907725ecb81b2d
MarcoFalke commented at 7:02 PM on February 19, 2018: member

@practicalswift Yes, the leaks are found by plain valgrind. The stack-use-after-scope by asan (requires clang) see https://clang.llvm.org/docs/AddressSanitizer.html#introduction
in src/bench/coin_selection.cpp:39 in fac85e5f94 outdated
```
  40 | -        // Empty wallet.
  41 | -        for (COutput output : vCoins)
  42 | -            delete output.tx;
  43 | -        vCoins.clear();
  44 | -
  45 | +    do {
```
laanwj commented at 6:27 PM on February 22, 2018:

I'm not sure I understand the rationale for going to while(...) to do {...} while(...) as used in all the other benchmarks. Say this function is called with state.KeepRunning() already zero, does it still need to do one iteration?

promag commented at 6:33 PM on February 22, 2018:

Right, just moving the cleanup below is enough right?
laanwj commented at 6:28 PM on February 22, 2018: member

Looks good apart from being confused about the while/do switch-around.
test: Plug memory leaks and stack-use-after-scope fadb39ca62
MarcoFalke force-pushed on Feb 22, 2018
MarcoFalke commented at 7:59 PM on February 22, 2018: member

Sorry, that do...while makes no sense.

Reverted.
promag commented at 8:04 PM on February 22, 2018: member

utACK fadb39c.
laanwj commented at 4:17 PM on February 23, 2018: member

utACK fadb39c
laanwj merged this on Feb 23, 2018
laanwj closed this on Feb 23, 2018
laanwj referenced this in commit acd1e6155c on Feb 23, 2018
MarcoFalke deleted the branch on Feb 23, 2018
PastaPastaPasta referenced this in commit 71f13b6025 on Jun 13, 2020
PastaPastaPasta referenced this in commit 0e94249afc on Jun 13, 2020
PastaPastaPasta referenced this in commit aa519cf6e8 on Jun 14, 2020
PastaPastaPasta referenced this in commit 0d90f3fd21 on Jun 17, 2020
PastaPastaPasta referenced this in commit 2907f846af on Jun 17, 2020
PastaPastaPasta referenced this in commit 63376cb7de on Jun 17, 2020
PastaPastaPasta referenced this in commit e6a1cc63d1 on Jun 17, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels