Page MenuHomePhabricator

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

Authored by Fabien on May 3 2019, 13:16.



Backport of core PR12477

The modifications in checkqueue_tests.cpp are already part of our

The modifications of test_bitcoin are only required to clear memory
from avalanche_tests and DoS_tests. Avalanche tests use
std::_unique_ptr, making this fix useless, and so does DoS tests
since D2919.

Depends on D2919

Test Plan

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 5662
Build 9386: Bitcoin ABC Buildbot (legacy)
Build 9385: arc lint + arc unit

Event Timeline

Fabien planned changes to this revision.May 3 2019, 14:18
Fabien edited the test plan for this revision. (Show Details)
jasonbcox requested changes to this revision.May 3 2019, 21:21
jasonbcox added a subscriber: jasonbcox.

Please still include the change in test_bitcoin.cpp. Although it appears useless, there's no telling if Core will write a new test in the future that depends on this, which we would then want to backport. Essentially, by leaving this change out, we are taking ownership of the code. Unless we also want to own net.h's definitions and switch vNodes to a vector of smart pointers, then I don't think this half-ownership is wise.

This revision now requires changes to proceed.May 3 2019, 21:21
Fabien requested review of this revision.May 4 2019, 09:47

The issue is that the modification in test_bitcoin is incompatible with avalanche_tests implementation and DoS_tests since D2919 (it leads to double free, crashing the tests).

These facilities were (before avalanche) specific to DoS_tests and are moved to the test file in

Because I found the solution of the unique_ptr more elegant I preferred to use this to update DoS_tests rather than removing it from avalanche_tests which I think would reduce the quality of the code.

Please review again with this input and share your thought.

deadalnix requested changes to this revision.May 4 2019, 17:41

And what about src/test/checkqueue_tests.cpp ?

This revision now requires changes to proceed.May 4 2019, 17:41
Fabien requested review of this revision.May 5 2019, 08:00

@deadalnix As mentioned in the summary, it has already been ported. You added the test in D1771 with the fix included.

This revision is now accepted and ready to land.May 5 2019, 18:01
This revision was automatically updated to reflect the committed changes.