Potential data race on fHavePruned flag #21627

1. 
   BradSwain commented at 8:58 pm on April 6, 2021: none

   We detected a potential race on the global variable fHavePruned with a static data race detection tool.

   Because the report is from a static analysis tool, we do not have a concrete execution or trace, but we have reviewed the report and are reasonably confident this variable can be accessed in parallel and is not guarded by any common locks.

   We are not able to confirm 100% that this race is real, or what impacts it may have on the program, but we decided to report it just to be safe.

   Writing Thread

   The flag can be set to true by the thread spawned at init.cpp:1883

   https://github.com/bitcoin/bitcoin/blob/9be7fe4849310884294669b019dd8300f69bc334/src/init.cpp#L1883-L1885

   This thread can call ThreadImport -> LoadExternalBlockFile -> AcceptBlock -> FlushStateToDisk

   And the FlushStateToDisk function can write to the fHavePruned flag.

   https://github.com/bitcoin/bitcoin/blob/9be7fe4849310884294669b019dd8300f69bc334/src/validation.cpp#L2311

   Reading Thread

   The fHavePruned flag can be read as part of the REST HTTP handler rest_block_extended which registered at rest.cpp:690.

   From there the handler can call rest_block -> blockToJSON -> IsBlockPruned

   And IsBlockPruned will read fHavePruned

   https://github.com/bitcoin/bitcoin/blob/9be7fe4849310884294669b019dd8300f69bc334/src/validation.h#L1026

   Expected behavior

   No threads reading and writing to the same variable without synchronization.

   Actual behavior

   It appears possible for threads to read and write fHavePruned in parallel.

   To reproduce

   May be difficult to reproduce consistently as we found the potential race through static analysis and data races can be non-deterministic.

   System information

   We analyzed commit 328aaac80 specifically, but the issue is still present at the time of posting this issue.

   For reference, here is a screenshot of the report generated by our tool (Coderrect Scanner). I have summarized all the info from this report above.

2. BradSwain added the label Bug on Apr 6, 2021
3. MarcoFalke added the label Block storage on Apr 6, 2021
4. 
   practicalswift commented at 6:05 am on April 9, 2021: contributor

   @BradSwain Thanks a lot for reporting!

   I love when our code base is analysed using new static analysis tools: the more the merrier! Did you find any other data race issues? Can you share your results in a gist or similar?

   Unfortunately we’re doing CI testing with quite a few ThreadSanitizer suppressions in https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/tsan. Were you able to replicate any of those finds?

   I think we should try to more clearly categorise our suppressions as “benign” vs. impactful: currently it is a mix of both which makes it hard to reason about the potential worst-case impact of each case.

   Last week a technical report on related work in the Firefox project was published: “Eliminating Data Races in Firefox – A Technical Report”. I think this makes sense:

   While benign data races do exist, we found (in agreement with previous work on this subject [1] [2]) that data races are very easily misclassified as benign. The reasons for this are clear: It is hard to reason about what compilers can and will optimize, and confirmation for certain “benign” data races requires you to look at the assembler code that the compiler finally produces.

   Needless to say, this procedure is often much more time consuming than fixing the actual data race and also not future-proof. As a result, we decided that the ultimate goal should be a “no data races” policy that declares even benign data races as undesirable due to their risk of misclassification, the required time for investigation and the potential risk from future compilers (with better optimizations) or future platforms (e.g. ARM).

5. 
   BradSwain commented at 5:30 pm on April 9, 2021: none

   Did you find any other data race issues? Were you able to replicate any of those finds?

   We are still going through some of the reports. We will see if our tool can also find those races from the TSan suppression list.
   I will post anything else we find as a comment on this issue.

   Last week a technical report on related work in the Firefox project was published

   I really like this report and I fully agree.

6. 
   amadeuszpawlik commented at 7:48 pm on June 9, 2021: contributor

   Don’t LoadExternalBlockFile and rest_block both hold the cs_main though?
7. 
   BradSwain commented at 8:22 pm on June 9, 2021: none

   Thanks for taking a look @amadeuszpawlik

   My understanding may be wrong, but is the lock released at the end of this block (line 263)?

   https://github.com/bitcoin/bitcoin/blob/9be7fe4849310884294669b019dd8300f69bc334/src/rest.cpp#L250-L263

8. 
   sipa commented at 9:07 pm on June 9, 2021: member

   @BradSwain Yes it is. LOCK(cs_main); locks for the duration of the scope it occurs in (like std::unique_lock etc).
9. 
   amadeuszpawlik commented at 8:54 am on June 10, 2021: contributor

   @BradSwain now I see, I was staring at rest_block -> IsBlockPruned but it obviously doesn’t hold for rest_block -> blockToJSON -> IsBlockPruned Thanks!
10. 
    MarcoFalke commented at 2:05 pm on June 10, 2021: member

    See also #17161
11. 
    pinheadmz commented at 4:46 pm on March 7, 2023: member

    I think this issue can be closed, fHavePruned seems to have been replaced with chainman().m_blockman.m_have_pruned (etc) and looks to me like all the setters (e.g. FlushStateToDisk()) have appropriate locks
12. 
    MarcoFalke commented at 4:52 pm on March 7, 2023: member

    The variable was only renamed, so the issue may still exist.
13. pinheadmz assigned pinheadmz on Jun 2, 2023

Contributors
BradSwain practicalswift amadeuszpawlik sipa MarcoFalke pinheadmz

Labels
Bug Block storage