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 merged this on May 28, 2018
16. laanwj closed this on May 28, 2018

    ---

17. laanwj referenced this in commit 14a4b49663 on May 28, 2018
18. MarcoFalke deleted the branch on May 29, 2018
19. MarcoFalke referenced this in commit d9c563095d on Jul 14, 2018
20. MarcoFalke added this to the milestone 0.16.2 on Jul 14, 2018
21. HashUnlimited referenced this in commit ecc7f47b82 on Jan 11, 2019
22. PastaPastaPasta referenced this in commit 5f8af115d6 on Jun 17, 2020
23. PastaPastaPasta referenced this in commit 1b7f452a0f on Jun 27, 2020
24. PastaPastaPasta referenced this in commit 0b4f9eace2 on Jun 28, 2020
25. PastaPastaPasta referenced this in commit ab83f73880 on Jun 29, 2020
26. PastaPastaPasta referenced this in commit a1c9327e0b on Jul 1, 2020
27. DrahtBot locked this on Sep 8, 2021

Contributors

MarcoFalkemruddyEmpactpromagjamesoblaanwj

Milestone
0.16.2