rpcban fixes #6307

Merged
merged 3 commits into from Jun 19, 2015

Conversation

Projects
None yet
2 participants
@jonasschnelli
Member

jonasschnelli commented Jun 19, 2015

  • fix a missing lock in clearban
  • add Ipv6 tests
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 19, 2015

Member

Travis error looks very serious, and occurs in the node handling test:

*** buffer overflow detected ***: /home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7faed05b9007]
/lib/x86_64-linux-gnu/libc.so.6(+0x107f00)[0x7faed05b7f00]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(__fdelt_chk+0x22)[0x7faed1ce2d25]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x2315e3)[0x7faed1b115e3]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(_Z11TraceThreadIPFvvEEvPKcT_+0x78)[0x7faed1b33f18]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x235adb)[0x7faed1b15adb]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x568294)[0x7faed1e48294]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x7e9a)[0x7faed0877e9a]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7faed05a24bd]
===== Memory map: ========
Member

laanwj commented Jun 19, 2015

Travis error looks very serious, and occurs in the node handling test:

*** buffer overflow detected ***: /home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7faed05b9007]
/lib/x86_64-linux-gnu/libc.so.6(+0x107f00)[0x7faed05b7f00]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(__fdelt_chk+0x22)[0x7faed1ce2d25]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x2315e3)[0x7faed1b115e3]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(_Z11TraceThreadIPFvvEEvPKcT_+0x78)[0x7faed1b33f18]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x235adb)[0x7faed1b15adb]
/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/src/bitcoind(+0x568294)[0x7faed1e48294]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x7e9a)[0x7faed0877e9a]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7faed05a24bd]
===== Memory map: ========
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 19, 2015

Member

Oh. Right. This is serious. I'm investigating...

Member

jonasschnelli commented Jun 19, 2015

Oh. Right. This is serious. I'm investigating...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 19, 2015

Member

Added a commit that fixes a race condition during node disconnecting.
Before this PR, we where using CNode::CloseSocketDisconnect() to disconnect nodes from the UI's peer table context menu as well over the rpc ban and disconnectnode functions. CNode::CloseSocketDisconnect() does close the socket directly, which is not thread safe because of missing locks around the socket handler.

This PR changes the disconnecting to go over the CNode::fDisconnect flag (which causes to disconnect the node during the existing node threads).

Member

jonasschnelli commented Jun 19, 2015

Added a commit that fixes a race condition during node disconnecting.
Before this PR, we where using CNode::CloseSocketDisconnect() to disconnect nodes from the UI's peer table context menu as well over the rpc ban and disconnectnode functions. CNode::CloseSocketDisconnect() does close the socket directly, which is not thread safe because of missing locks around the socket handler.

This PR changes the disconnecting to go over the CNode::fDisconnect flag (which causes to disconnect the node during the existing node threads).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 19, 2015

Member

Tested ACK.
I would call this "fix" not "overhaul".

Member

laanwj commented Jun 19, 2015

Tested ACK.
I would call this "fix" not "overhaul".

@jonasschnelli jonasschnelli changed the title from rpcban overhaul to rpcban fixes Jun 19, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 19, 2015

Member

Agreed. Changed PR title.

Member

jonasschnelli commented Jun 19, 2015

Agreed. Changed PR title.

@laanwj laanwj merged commit 1c043d5 into bitcoin:master Jun 19, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jun 19, 2015

Merge pull request #6307
1c043d5 fix lock issue for QT node diconnect and RPC disconnectnode (Jonas Schnelli)
932687b setban: add IPv6 tests (Jonas Schnelli)
62909f6 fix missing lock in CNode::ClearBanned() (Jonas Schnelli)

@str4d str4d referenced this pull request in zcash/zcash Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment