Skip to content

net: Add missing lock in ProcessHeadersMessage(...) #11578

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Oct 31, 2017
Merged

net: Add missing lock in ProcessHeadersMessage(...) #11578

laanwj merged 1 commit into from
Oct 31, 2017

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 30, 2017

Add missing lock in ProcessHeadersMessage(...).

Reading the variable mapBlockIndex requires holding the mutex cs_main.

The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of #11568 two days ago did not lock cs_main prior to accessing mapBlockIndex.

@fanquake fanquake added the P2P label Oct 30, 2017
@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 30, 2017

Friendly ping @sdaftuar: does this look correct? :-)

sdaftuar
sdaftuar reviewed Oct 30, 2017
View reviewed changes
Copy link
Member

@sdaftuar sdaftuar left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! We should tag for backport.

Left a small suggestion for simplifying that block a little.

src/net_processing.cpp Outdated
Copy link
Member

@sdaftuar sdaftuar Oct 30, 2017

Choose a reason for hiding this comment

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

I think we can move this up to above the if (nDoS > 0) line, and then remove the LOCK(cs_main) that's inside that block?

@promag
Copy link
Contributor

promag commented Oct 30, 2017 •
edited
Loading

One of the calls already has the lock, should instead assert the lock is held and add the other lock before?

bitcoin/src/net_processing.cpp

Line 2524 in bb9ab0f

return ProcessHeadersMessage(pfrom, connman, headers, chainparams, should_punish);

Edit: sorry, after all none of the calls have the lock.

@sdaftuar
Copy link
Member

sdaftuar commented Oct 30, 2017

@promag I think neither caller of ProcessHeadersMessage() is holding cs_main (and they shouldn't be, either).

@promag
Copy link
Contributor

promag commented Oct 30, 2017

Maybe ProcessNewBlockHeaders should return the first invalid block index instead? (not the same as the fist invalid block header)

@promag
Copy link
Contributor

promag commented Oct 30, 2017

@promag I think neither caller of ProcessHeadersMessage() is holding cs_main (and they shouldn't be, either).

@sdaftuar yes, missed a } while scrolling the code.

@sdaftuar
Copy link
Member

sdaftuar commented Oct 30, 2017

@promag The first invalid block header may not have been added to mapBlockIndex.

@promag
Copy link
Contributor

promag commented Oct 30, 2017

Right. Anyway, something like #11041 can easily prevent these cases.

@laanwj laanwj added this to the 0.15.0.2 milestone Oct 30, 2017
@practicalswift
net: Add missing lock in ProcessHeadersMessage(...)
2530bf2 
Reading the variable mapBlockIndex requires holding the mutex cs_main.

The new "Disconnect outbound peers relaying invalid headers" code
added in commit 37886d5 and merged
as part of bitcoin#11568 two days ago did not lock cs_main prior to accessing
mapBlockIndex.
@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 30, 2017

@sdaftuar Thanks for the review. The two locks are now merged into one as suggested :-)

@achow101
Copy link
Member

achow101 commented Oct 30, 2017

utACK 2530bf2

sdaftuar
sdaftuar approved these changes Oct 30, 2017
View reviewed changes
Copy link
Member

@sdaftuar sdaftuar left a comment

Choose a reason for hiding this comment

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

utACK 2530bf2

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Oct 30, 2017

utACK 2530bf2

@laanwj laanwj merged commit 2530bf2 into bitcoin:master Oct 31, 2017
laanwj added a commit that referenced this pull request Oct 31, 2017
@laanwj
Merge #11578: net: Add missing lock in ProcessHeadersMessage(...)
8335cb4 
2530bf2 net: Add missing lock in ProcessHeadersMessage(...) (practicalswift)

Pull request description:

  Add missing lock in `ProcessHeadersMessage(...)`.

  Reading the variable `mapBlockIndex` requires holding the mutex `cs_main`.

  The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of #11568 two days ago did not lock `cs_main` prior to accessing `mapBlockIndex`.

Tree-SHA512: b799c234be8043d036183a00bc7867bbf3bd7ffe3baa94c88529da3b3cd0571c31ed11dadfaf29c5b8498341d6d0a3c928029a43b69f3267ef263682c91563a3
@laanwj
Copy link
Member

laanwj commented Nov 1, 2017

This should be backported after #11568.

@morcos morcos mentioned this pull request Nov 2, 2017
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017
@practicalswift
net: Add missing lock in ProcessHeadersMessage(...)
ec8dedf 
Reading the variable mapBlockIndex requires holding the mutex cs_main.

The new "Disconnect outbound peers relaying invalid headers" code
added in commit 37886d5 and merged
as part of bitcoin#11568 two days ago did not lock cs_main prior to accessing
mapBlockIndex.

Github-Pull: bitcoin#11578
Rebased-From: 2530bf2
@practicalswift practicalswift mentioned this pull request Apr 30, 2018
Closed
30 tasks
codablock pushed a commit to codablock/dash that referenced this pull request Sep 29, 2019
@laanwj @codablock
Merge bitcoin#11578: net: Add missing lock in ProcessHeadersMessage(...)
c743111 
2530bf2 net: Add missing lock in ProcessHeadersMessage(...) (practicalswift)

Pull request description:

  Add missing lock in `ProcessHeadersMessage(...)`.

  Reading the variable `mapBlockIndex` requires holding the mutex `cs_main`.

  The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of bitcoin#11568 two days ago did not lock `cs_main` prior to accessing `mapBlockIndex`.

Tree-SHA512: b799c234be8043d036183a00bc7867bbf3bd7ffe3baa94c88529da3b3cd0571c31ed11dadfaf29c5b8498341d6d0a3c928029a43b69f3267ef263682c91563a3
@practicalswift practicalswift deleted the ProcessHeadersMessage branch April 10, 2021 19:33
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@sdaftuar sdaftuar sdaftuar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

0.15.1

Development

Successfully merging this pull request may close these issues.

8 participants

@practicalswift @promag @sdaftuar @achow101 @TheBlueMatt @laanwj @fanquake @maflcko