Skip to content

Conversation

theuni
Copy link
Member

@theuni theuni commented Sep 13, 2016

This fixes a possible shutdown crash. Thanks to @morcos for reporting.

In the case of (for example) an already-running bitcoind, the shutdown sequence begins before CConnman has been created, leading to a null-pointer dereference when g_connman->Stop() is called.

@sipa
Copy link
Member

sipa commented Sep 13, 2016

Concept ACK, but wouldn't it be easier to invoke CConnman::Stop() from CConnman::~CConnman, and then call g_connman.reset() at the time you'd stop?

@theuni
Copy link
Member Author

theuni commented Sep 13, 2016

As discussed on IRC, yes. This is over-engineered as-is, for future threading scenarios that may not ever happen. I'll make that change.

In the case of (for example) an already-running bitcoind, the shutdown sequence
begins before CConnman has been created, leading to a null-pointer dereference
when g_connman->Stop() is called.

Instead, Just let the CConnman dtor take care of stopping.
@theuni theuni force-pushed the fix-connman-shutdown branch from f7b298b to 36fa01f Compare September 14, 2016 02:44
@theuni
Copy link
Member Author

theuni commented Sep 14, 2016

Replaced with the simpler version.

@laanwj
Copy link
Member

laanwj commented Sep 14, 2016

utACK 36fa01f

@laanwj laanwj merged commit 36fa01f into bitcoin:master Sep 14, 2016
laanwj added a commit that referenced this pull request Sep 14, 2016
36fa01f net: only delete CConnman if it's been created (Cory Fields)
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 27, 2020
e733939 [Cleanup] Remove CConnman::Copy(Release)NodeVector, now unused (random-zebra)
b09da57 [Refactor] Proper CConnman encapsulation of mnsync (random-zebra)
219061e [Refactor] Decouple SyncWithNode from CMasternodeSync::Process() (random-zebra)
e1f3620 [Refactor] Proper CConnman encapsulation of proposals / votes sync (random-zebra)
f38c9f9 miner: don't try to access connman if it doesn't exist (Fuzzbawls)
92ab619 Address feedback (Fuzzbawls)
9d5346a Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
fe12ff9 net: No longer send local address in addrMe (Wladimir J. van der Laan)
14d6917 net: misc header cleanups (Cory Fields)
34ba0de net: make proxy receives interruptible (Cory Fields)
1bd97ef net: make net processing interruptable (Fuzzbawls)
e24b4cc net: make net interruptible (Cory Fields)
814d0de net: add CThreadInterrupt and InterruptibleSleep (Cory Fields)
1443f2a net: a few small cleanups before replacing boost threads (Cory Fields)
58eabe4 net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields)
874baba Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin)
9485ab0 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin)
c1e59ad minor net cleanups (Fuzzbawls)
07ae004 net: move vNodesDisconnected into CConnman (Cory Fields)
276c946 net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields)
22a9aff net: Introduce CConnection::Options to avoid passing so many params (Cory Fields)
e4891bf net: Drop StartNode/StopNode and use CConnman directly (Cory Fields)
431575c net: pass CClientUIInterface into CConnman (Cory Fields)
48de47e net: Pass best block known height into CConnman (Cory Fields)
15eed91 net: move max/max-outbound to CConnman (Cory Fields)
2bf0921 net: move semOutbound to CConnman (Cory Fields)
481929f net: move nLocalServices/nRelevantServices to CConnman (Cory Fields)
bcee6ae net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields)
6865469 net: move send/recv statistics to CConnman (Cory Fields)
1cec418 net: SocketSendData returns written size (Cory Fields)
2bb9dfa net: move messageHandlerCondition to CConnman (Cory Fields)
9c5a0df net: move nLocalHostNonce to CConnman (Cory Fields)
a1394ef net: move nLastNodeId to CConnman (Cory Fields)
3804c29 net: move whitelist functions into CConnman (Cory Fields)
dbde9be net: create generic functor accessors and move vNodes to CConnman (Cory Fields)
2e02467 net: Add most functions needed for vNodes to CConnman (Cory Fields)
5667e61 net: move added node functions to CConnman (Cory Fields)
37487ed net: Add oneshot functions to CConnman (Cory Fields)
facf878 net: move ban and addrman functions into CConnman (Cory Fields)
091aaf2 net: handle nodesignals in CConnman (Cory Fields)
1e9fa0f net: move OpenNetworkConnection into CConnman (Cory Fields)
573200f net: Move socket binding into CConnman (Cory Fields)
7762b97 net: Pass CConnection to wallet rather than using the global (Fuzzbawls)
00591b8 net: Pass CConnman around as needed (Cory Fields)
2cd3d39 net: Add rpc error for missing/disabled p2p functionality (Cory Fields)
f08e316 net: Create CConnman to encapsulate p2p connections (Cory Fields)
66337dc net: move CBanDB and CAddrDB out of net.h/cpp (Fuzzbawls)
10969f6 gui: add NodeID to the peer table (Cory Fields)
58044ac Fix some locks (Pieter Wuille)
f9f8926 Do not shadow variables in networking code (Pavel Janík)

Pull request description:

  This is the finalization of the upstream PRs being tracked in #1374 to update much of our networking code to more closely follow upstream improvements. The following PRs are included here:

  - bitcoin#8466
    Do not shadow variables in networking code
  - bitcoin#8606
    Fix some locks
  - bitcoin#8085
    Begin encapsulation
  - bitcoin#9289
    drop boost::thread_group
  - bitcoin#8740
    No longer send local address in addrMe
  - bitcoin#8661
    Do not set an addr time penalty when a peer advertises itself

  Additionally, during conflict resolution and backporting of 8085, the following additional upstream PR was included:
  - bitcoin#8715
    only delete CConnman if it's been created

  Still TODO in future PRs:
  - bitcoin#9037
  - bitcoin#9441
  - bitcoin#9609
  - bitcoin#9626
  - bitcoin#9708
  - bitcoin#12381
  - bitcoin#18584

ACKs for top commit:
  furszy:
    same here, code ACK e733939. Nice work ☕ .
  furszy:
    ACK e733939 .
  random-zebra:
    ACK e733939 and merging...

Tree-SHA512: 0fc3cca76d9ddc13f75fc9d48c48d215e6c0e0381377c0318436176a0e0ead73b511e0061a4b63f7052874730b6da8b0ffc2c94cba034bcc39aad4212b69ee22
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants