qa: Initialize lockstack to prevent null pointer deref #13300

1. 
   MarcoFalke commented at 1:28 AM on May 22, 2018: member

   It is currently impossible to call debug methods such as AssertLock(Not)Held on a thread without running into undefined behavior, unless a lock was pushed on the stack in this thread.

   Initializing the global lockstack seems to fix both issues.

2. MarcoFalke added the label Tests on May 22, 2018
3. 
   MarcoFalke commented at 1:29 AM on May 22, 2018: member

   Fixes: #13263

4. 
   mruddy commented at 1:58 AM on May 22, 2018: contributor

   Nice, thanks for this fix. I don't feel qualified to review the code change itself. But, I did verify that the patch appears to fix #13263 for me too. Thanks!

5. 
   Empact commented at 2:37 AM on May 22, 2018: member

   If it's a static variable initialized at startup, maybe do away with the pointer?

6. in src/sync.cpp:78 in fa6a9b5088 outdated

     74 | @@ -75,7 +75,7 @@ struct LockData {
     75 |      std::mutex dd_mutex;
     76 |  } static lockdata;
     77 |  
     78 | -static thread_local std::unique_ptr<LockStack> lockstack;
     79 | +static thread_local std::unique_ptr<LockStack> lockstack{new LockStack};
   

   ---

   ---

   

   promag commented at 9:25 AM on May 22, 2018:

   I guess MakeUnique/std::make_unique is not that relevant here.

   ---

   

   promag commented at 9:30 AM on May 22, 2018:

   Also, const static?

7. 
   promag commented at 9:30 AM on May 22, 2018: member

   @Empact that should work but maybe consider this patch as a "fix". Also keeping it as a smart pointer is more flexible IMO.

   utACK fa6a9b5.

8. qa: Initialize lockstack to prevent null pointer deref fa9da85b7c
9. MarcoFalke force-pushed on May 22, 2018
10. 
    promag commented at 1:24 PM on May 22, 2018: member

    utACK fa9da85, statically allocated does look better. Not sure about the new variable name - is a static variable also global?

11. 
    jamesob commented at 1:50 PM on May 22, 2018: member

    utACK https://github.com/bitcoin/bitcoin/pull/13300/commits/fa9da85b7cc759d06bc24854be2bad0ea87b6006

12. 
    MarcoFalke commented at 1:51 PM on May 22, 2018: member

    @promag My rule of thumb is "everything is a global unless it is a class member". Couldn't yet find any issues with that rule :p

13. 
    Empact commented at 5:32 AM on May 23, 2018: member

    utACK fa9da85

14. 
    laanwj commented at 2:16 PM on May 28, 2018: member

    Thread-local is a very special kind of global, but as for variable naming it's fine, it has global scope from the perspective of the compiler.

    utACK fa9da85b7cc759d06bc24854be2bad0ea87b6006

15. laanwj cross-referenced this on May 28, 2018 from issue tests: make check failure by mruddy
16. laanwj merged this on May 28, 2018
17. laanwj closed this on May 28, 2018

    ---

18. laanwj referenced this in commit 14a4b49663 on May 28, 2018
19. Bushstar cross-referenced this on May 29, 2018 from issue commits from bitcoin/master by Bushstar
20. MarcoFalke deleted the branch on May 29, 2018
21. MarcoFalke referenced this in commit d9c563095d on Jul 14, 2018
22. MarcoFalke added this to the milestone 0.16.2 on Jul 14, 2018
23. HashUnlimited referenced this in commit ecc7f47b82 on Jan 11, 2019
24. AkioNak cross-referenced this on Jan 28, 2019 from issue memory: Construct globals on first use by MarcoFalke
25. PastaPastaPasta referenced this in commit 5f8af115d6 on Jun 17, 2020
26. PastaPastaPasta referenced this in commit 1b7f452a0f on Jun 27, 2020
27. PastaPastaPasta referenced this in commit 0b4f9eace2 on Jun 28, 2020
28. PastaPastaPasta referenced this in commit ab83f73880 on Jun 29, 2020
29. PastaPastaPasta referenced this in commit a1c9327e0b on Jul 1, 2020
30. str4d cross-referenced this on Apr 12, 2021 from issue Sync backports by str4d
31. bitcoin locked this on Sep 8, 2021

Contributors

MarcoFalkemruddyEmpactpromagjamesoblaanwj

Milestone
0.16.2