Page MenuHomePhabricator

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

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

Details

Summary

Backport of core PR12477
https://github.com/bitcoin/bitcoin/pull/12477/files

The modifications in checkqueue_tests.cpp are already part of our
codebase.

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
./src/bench/bitcoin-bench

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR12477
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5667
Build 9396: Bitcoin ABC Buildbot (legacy)
Build 9395: 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 https://github.com/bitcoin/bitcoin/commit/136bd7926c72659dd277a7b795ea17f72e523338#diff-d5ba361c5f8be78eb4cc0c787c1fc78e.

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.