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

fix race that could fail to persist a ban #7959

Merged
merged 1 commit into from May 2, 2016

Conversation

Projects
None yet
4 participants
@kazcw
Contributor

kazcw commented Apr 27, 2016

DumpBanList currently does this:

  • with lock: take a copy of the banmap
  • perform I/O (write out the banmap)
  • with lock: mark the banmap non-dirty

If a new ban is added during the I/O operation, it may never be persisted to disk.

Reorder operations so that the data to be persisted cannot be older than the time at which the banmap was marked non-dirty.

fix race that could fail to persist a ban
DumpBanList currently does this:
  - with lock: take a copy of the banmap
  - perform I/O (write out the banmap)
  - with lock: mark the banmap non-dirty
If a new ban is added during the I/O operation, it may never be persisted to
disk.

Reorder operations so that the data to be persisted cannot be older than the
time at which the banmap was marked non-dirty.
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 27, 2016

Member

ut ACK f4ac02e

Member

theuni commented Apr 27, 2016

ut ACK f4ac02e

@jonasschnelli jonasschnelli added the P2P label Apr 28, 2016

@@ -2634,9 +2634,10 @@ void DumpBanlist()
CBanDB bandb;
banmap_t banmap;
CNode::SetBannedSetDirty(false);

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 28, 2016

Member

What about doing the whole bandb.Write(ban map) & CNode::SetBannedSetDirty(false); while holding LOCK(cs_setBanned);?
Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).

@jonasschnelli

jonasschnelli Apr 28, 2016

Member

What about doing the whole bandb.Write(ban map) & CNode::SetBannedSetDirty(false); while holding LOCK(cs_setBanned);?
Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).

This comment has been minimized.

@laanwj

laanwj Apr 28, 2016

Member

Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).

That wouldn't be a race condition, but the way it's structured on purpose: because the change to the ban map will re-mark the map as dirty, the next write will catch it (there is a very slight window between CNode::SetBannedSetDirty and CNode::GetBanned in which a false-positive dirty could happen, but unlike the other way around that's harmless). This is not the case right now.

Putting it in a huge lock would hold up the entire P2P for the time of the I/O, which would be unfortunate.

@laanwj

laanwj Apr 28, 2016

Member

Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).

That wouldn't be a race condition, but the way it's structured on purpose: because the change to the ban map will re-mark the map as dirty, the next write will catch it (there is a very slight window between CNode::SetBannedSetDirty and CNode::GetBanned in which a false-positive dirty could happen, but unlike the other way around that's harmless). This is not the case right now.

Putting it in a huge lock would hold up the entire P2P for the time of the I/O, which would be unfortunate.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 28, 2016

Member

utACK f4ac02e

Member

laanwj commented Apr 28, 2016

utACK f4ac02e

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Apr 29, 2016

utACK f4ac02e

@laanwj laanwj merged commit f4ac02e into bitcoin:master May 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 2, 2016

Merge #7959: fix race that could fail to persist a ban
f4ac02e fix race that could fail to persist a ban (Kaz Wesley)

@kazcw kazcw deleted the kazcw:banmap_race branch May 5, 2016

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