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.

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

[Bug] Fix validation interface data race #2924

Merged

Conversation

panleone
Copy link

@panleone panleone commented Apr 6, 2024

first commit:
Partially backport bitcoin PR bitcoin#18338 which introduce the functions
RegisterSharedValidationInterface and UnregisterSharedValidationInterface. See that PR why using normal pointers is problematic, and how shared_ptr solve the issue
(In a few words the problem is that it can happen that the pointed memory is freed before all signals have been processed, while with shared pointer we are sure that memory will be freed after the last signal is handled)

second commit:
Utilize the new functions to the validation interface BlockStateCatcher. In order to keep everywhere the logic unchanged (Registering on creation and Unregistering when the object goes out of scope) I created the wrapper class BlockStateCatcherWrapper which contains a shared_ptr to BlockStatecatcher.

Those two commits solve the following data race where a BlockStateCatcher pointer is dereferenced after the pointed memory is freed

WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=6423)
  Write of size 8 at 0x7fc3d7c2d570 by thread T20:
    #0 CValidationInterface::~CValidationInterface() validationinterface.h:75 (pivxd+0xeacac)
    #1 BlockStateCatcher::~BlockStateCatcher() util/blockstatecatcher.h:23 (pivxd+0xeacac)
    #2 generateBlocks(Consensus::Params const&, CWallet*, bool, int, int, int, CScript*) rpc/mining.cpp:78 (pivxd+0x1c1a98)
    #3 generate(JSONRPCRequest const&) rpc/mining.cpp:140 (pivxd+0x1c2215)
    ...

  Previous read of size 8 at 0x7fc3d7c2d570 by thread T35 (mutexes: write M133270, write M132847):
    #0 void std::__invoke_impl<void, void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>(std::__invoke_memfun_deref, void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*&&) /usr/include/c++/12/bits/invoke.h:74 (pivxd+0x2eef14)
    #1 std::__invoke_result<void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>::type std::__invoke<void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>(void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*&&) /usr/include/c++/12/bits/invoke.h:96 (pivxd+0x2eefbb)
    #2 void std::_Bind<void (CValidationInterface::*(CValidationInterface*, std::_Placeholder<1>))(CConnman*)>::__call<void, CConnman*&&, 0ul, 1ul>(std::tuple<CConnman*&&>&&, std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/functional:484 (pivxd+0x2eefbb)
    ...

@panleone panleone added the Bug label Apr 6, 2024
@panleone panleone added this to the 6.0.0 milestone Apr 6, 2024
@panleone panleone self-assigned this Apr 6, 2024
@panleone panleone force-pushed the fix_validation_interface_data_race branch from de59666 to ddbd4b0 Compare April 7, 2024 11:03
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

utACK ddbd4b0

Copy link
Member

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK ddbd4b0

@Fuzzbawls Fuzzbawls merged commit 4469fa0 into PIVX-Project:master Apr 20, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants