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

1. 
   practicalswift commented at 1:36 pm on October 30, 2017: contributor

   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 37886d5e2f9992678dea4b1bd893f4f10d61d3ad and merged as part of #11568 two days ago did not lock cs_main prior to accessing mapBlockIndex.

2. fanquake added the label P2P on Oct 30, 2017
3. 
   practicalswift commented at 1:51 pm on October 30, 2017: contributor

   Friendly ping @sdaftuar: does this look correct? :-)
4. in src/net_processing.cpp:1269 in 042831e741 outdated

   1265@@ -1266,6 +1266,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
   1266                 LOCK(cs_main);
   1267                 Misbehaving(pfrom->GetId(), nDoS);
   1268             }
   1269+            LOCK(cs_main);
   

   ---

   ---

   

   sdaftuar commented at 2:16 pm on October 30, 2017:

   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?
5. 
   sdaftuar commented at 2:19 pm on October 30, 2017: member

   Thanks for catching this! We should tag for backport.

   Left a small suggestion for simplifying that block a little.

6. 
   promag commented at 2:20 pm on October 30, 2017: member

   One of the calls already has the lock, should instead assert the lock is held and add the other lock before? https://github.com/bitcoin/bitcoin/blob/bb9ab0fccfbadd5c032a2cd0bb3135049cffa42b/src/net_processing.cpp#L2524

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

7. 
   sdaftuar commented at 2:25 pm on October 30, 2017: member

   @promag I think neither caller of ProcessHeadersMessage() is holding cs_main (and they shouldn’t be, either).
8. 
   promag commented at 2:33 pm on October 30, 2017: member

   Maybe ProcessNewBlockHeaders should return the first invalid block index instead? (not the same as the fist invalid block header)
9. 
   promag commented at 2:34 pm on October 30, 2017: member

   @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.

10. 
    sdaftuar commented at 2:35 pm on October 30, 2017: member

    @promag The first invalid block header may not have been added to mapBlockIndex.
11. 
    promag commented at 2:46 pm on October 30, 2017: member

    Right. Anyway, something like #11041 can easily prevent these cases.
12. laanwj added this to the milestone 0.15.0.2 on Oct 30, 2017
13. net: 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 37886d5e2f9992678dea4b1bd893f4f10d61d3ad and merged
    as part of #11568 two days ago did not lock cs_main prior to accessing
    mapBlockIndex.

    2530bf27b7
14. practicalswift force-pushed on Oct 30, 2017
15. 
    practicalswift commented at 7:01 pm on October 30, 2017: contributor

    @sdaftuar Thanks for the review. The two locks are now merged into one as suggested :-)
16. 
    achow101 commented at 7:03 pm on October 30, 2017: member

    utACK 2530bf27b72e53cc6ffec27de35f3b487984833d
17. sdaftuar approved
18. 
    sdaftuar commented at 7:07 pm on October 30, 2017: member

    utACK 2530bf27b72e53cc6ffec27de35f3b487984833d
19. 
    TheBlueMatt commented at 11:06 pm on October 30, 2017: member

    utACK 2530bf27b72e53cc6ffec27de35f3b487984833d
20. laanwj merged this on Oct 31, 2017
21. laanwj closed this on Oct 31, 2017

    ---

22. laanwj referenced this in commit 8335cb4781 on Oct 31, 2017
23. MarcoFalke added the label Needs backport on Oct 31, 2017
24. 
    laanwj commented at 8:06 pm on November 1, 2017: member

    This should be backported after #11568.
25. MarcoFalke removed the label Needs backport on Nov 1, 2017
26. MarcoFalke referenced this in commit 3ea950d215 on Nov 1, 2017
27. MarcoFalke referenced this in commit ec8dedff46 on Nov 2, 2017
28. codablock referenced this in commit 39cf1d3944 on Sep 26, 2019
29. codablock referenced this in commit c743111c66 on Sep 29, 2019
30. barrystyle referenced this in commit 3ac71cebf3 on Jan 22, 2020
31. practicalswift deleted the branch on Apr 10, 2021
32. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift sdaftuar promag achow101 TheBlueMatt laanwj

Labels
P2P

Milestone
0.15.1