Potential data race on fHavePruned flag #21627

<!-- This issue tracker is only for technical issues related to Bitcoin Core. General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com. For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/. If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

<!-- Describe the issue -->

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.

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

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.

Don't LoadExternalBlockFile and rest_block both hold the cs_main though?

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

@BradSwain Yes it is. LOCK(cs_main); locks for the duration of the scope it occurs in (like std::unique_lock etc).

@BradSwain now I see, I was staring at rest_block -> IsBlockPruned but it obviously doesn't hold for rest_block -> blockToJSON -> IsBlockPruned Thanks!

See also #17161

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

The variable was only renamed, so the issue may still exist.

@pinheadmz are you still looking into this? Seems it's been a while since any progress has been made to it? Is it still relevant?

I haven't been focusing on this, wanna take a stab at it?

Sure, I can give it a go