Cross Translation Unit (CTU) Static Analysis with Clang #17454

Dear Bitcoin Community,

Clang's CTU static analysis has evolved a lot in this year. We have chosen Bitcoin as one of the projects which we continuously analyze in our CI loop to safeguard the quality of CTU analysis. We thought that the results might be interesting for you Bitcoin developers too. So, we stored the results for one run on a demo server for version 0.18.1. Please use demo/demo to log in. If you find any false positive or if you have any other comments then we would be happy to receive your feedback.

Sincerely, Gabor Marton (CTU developer)

@dkrupp @gyorb

Oh, interesting!

@martong That is some excellent news! Very happy to hear this!

Can you post the 0.18.1 warnings here or in a gist too? :)

Ignoring the warnings in third party libraries, just some notes about the remaining ones in our code:

• chainActive.Tip() is generally assumed to be not null (at the very least it should point to the genesis block). The genesis block initialization happens in init.cpp, but might be hard for CTU to pick up.
• nMinerConfirmationWindow is never assigned a value of zero, so divide by zero can't really happen. (See $ git grep 'nMinerConfirmationWindow =')
• Haven't looked at the pBestIndex null dereference in net processing.

@martong That is some excellent news! Very happy to hear this!

Can you post the 0.18.1 warnings here or in a gist too? :)

Yes, I hope this table is readable. However, to understand the path sensitive warning, one must take a look at the path in the web browser anyway.

File                                                                                                                      | Checker                            | Severity | Message                                                                                                                 | Bug path length | Review status | Detection status
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/usr/include/boost/date_time/int_adapter.hpp @ 347                                                                        | core.DivideZero                    | HIGH     | Division by zero                                                                                                        |              15 | UNREVIEWED    | NEW
/usr/include/boost/concept/detail/general.hpp @ 39                                                                        | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |               1 | UNREVIEWED    | NEW
/usr/include/boost/concept/detail/general.hpp @ 47                                                                        | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |               1 | UNREVIEWED    | NEW
/usr/include/boost/concept/usage.hpp @ 16                                                                                 | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/net_processing.cpp @ 3480             | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |              55 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/db/memtable.cc @ 18           | core.CallAndMessage                | HIGH     | 2nd function call argument is an uninitialized value                                                                    |              20 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/validation.cpp @ 4075                 | core.NullDereference               | HIGH     | Access to field 'nHeight' results in a dereference of a null pointer (loaded from variable 'pindex')                    |              17 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/versionbits.cpp @ 109                 | core.DivideZero                    | HIGH     | Division by zero                                                                                                        |              44 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/crypto/common.h @ 66                  | core.NonNullParamChecker           | HIGH     | Null pointer passed to 2nd parameter expecting 'nonnull'                                                                |              15 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/util.h @ 24             | core.NullDereference               | HIGH     | Access to field 'fn' results in a dereference of a null pointer (loaded from variable 'cb')                             |               6 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/ecmult_impl.h @ 217     | core.NullDereference               | HIGH     | Access to field 'pre_g' results in a dereference of a null pointer (loaded from variable 'ctx')                         |               5 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/ecmult_gen_impl.h @ 95  | core.NullDereference               | HIGH     | Access to field 'prec' results in a dereference of a null pointer (loaded from variable 'ctx')                          |               5 | UNREVIEWED    | NEW
/usr/include/boost/multi_index/detail/ord_index_node.hpp @ 306                                                            | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |              16 | UNREVIEWED    | NEW
/usr/include/boost/multi_index/detail/ord_index_node.hpp @ 312                                                            | core.CallAndMessage                | HIGH     | Called C++ object pointer is null                                                                                       |              17 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/validation.cpp @ 4075                 | core.NullDereference               | HIGH     | Access to field 'nHeight' results in a dereference of a null pointer (loaded from variable 'pindex')                    |              13 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/db/memtable.cc @ 18           | core.CallAndMessage                | HIGH     | 2nd function call argument is an uninitialized value                                                                    |              15 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/util/crc32c.cc @ 333          | core.UndefinedBinaryOperatorResult | MEDIUM   | The right operand of '^' is a garbage value                                                                             |              30 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/util.h @ 72             | optin.portability.UnixAPI          | MEDIUM   | Call to 'malloc' has an allocation size of 0 bytes                                                                      |               6 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/field_5x52_impl.h @ 377 | core.UndefinedBinaryOperatorResult | MEDIUM   | The right operand of '-' is a garbage value                                                                             |              11 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/field_5x52_impl.h @ 390 | core.uninitialized.Assign          | MEDIUM   | The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage       |              16 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/field_5x52_impl.h @ 406 | core.uninitialized.Assign          | MEDIUM   | Assigned value is garbage or undefined                                                                                  |              11 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/field_5x52_impl.h @ 406 | core.uninitialized.Assign          | MEDIUM   | The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage       |              25 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/scalar_4x64_impl.h @ 60 | core.UndefinedBinaryOperatorResult | MEDIUM   | The left operand of '<' is a garbage value                                                                              |               7 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/util/crc32c.cc @ 333          | core.UndefinedBinaryOperatorResult | MEDIUM   | The right operand of '^' is a garbage value                                                                             |              27 | UNREVIEWED    | NEW
/usr/include/c++/6/bits/atomic_base.h @ 188                                                                               | deadcode.DeadStores                | LOW      | Value stored to '__b' during its initialization is never read                                                           |               1 | UNREVIEWED    | NEW
/usr/include/c++/6/bits/atomic_base.h @ 199                                                                               | deadcode.DeadStores                | LOW      | Value stored to '__b' during its initialization is never read                                                           |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/netbase.cpp @ 350                     | deadcode.DeadStores                | LOW      | Although the value stored to 'recvr' is used in the enclosing expression, the value is never actually read from 'recvr' |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/netbase.cpp @ 373                     | deadcode.DeadStores                | LOW      | Although the value stored to 'recvr' is used in the enclosing expression, the value is never actually read from 'recvr' |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/netbase.cpp @ 439                     | deadcode.DeadStores                | LOW      | Although the value stored to 'recvr' is used in the enclosing expression, the value is never actually read from 'recvr' |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/secp256k1/src/ecmult_gen_impl.h @ 153 | deadcode.DeadStores                | LOW      | Value stored to 'bits' is never read                                                                                    |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/crypto/sha256.cpp @ 598               | deadcode.DeadStores                | LOW      | Value stored to 'enabled_avx' is never read                                                                             |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/crypto/sha256.cpp @ 602               | deadcode.DeadStores                | LOW      | Value stored to 'have_avx2' is never read                                                                               |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/crypto/sha256.cpp @ 603               | deadcode.DeadStores                | LOW      | Value stored to 'have_shani' is never read                                                                              |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/db/log_reader.cc @ 42         | deadcode.DeadStores                | LOW      | Value stored to 'offset_in_block' is never read                                                                         |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/db/log_reader.cc @ 103        | deadcode.DeadStores                | LOW      | Value stored to 'in_fragmented_record' is never read                                                                    |               1 | UNREVIEWED    | NEW
/var/jenkins_home/workspace/ctu_pipeline_clang-master-monorepo/projects/Bitcoin/src/leveldb/db/log_reader.cc @ 121        | deadcode.DeadStores                | LOW      | Value stored to 'in_fragmented_record' is never read                                                                    |               1 | UNREVIEWED    | NEW
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Ignoring the warnings in third party libraries, just some notes about the remaining ones in our code:

• chainActive.Tip() is generally assumed to be not null (at the very least it should point to the genesis block). The genesis block initialization happens in init.cpp, but might be hard for CTU to pick up.
• nMinerConfirmationWindow is never assigned a value of zero, so divide by zero can't really happen. (See $ git grep 'nMinerConfirmationWindow =')
• Haven't looked at the pBestIndex null dereference in net processing.

Thank you for your feedback. We are going to investigate the false positive results and we are going to find out if we can improve the analyzer to avoid similar false positives in the future.

For reference, the check that the tip is never nullptr (aka at least the genesis block) in init is:

https://github.com/bitcoin/bitcoin/blob/6d4b97cb1c21f9534b01fad037837b7966b82190/src/init.cpp#L1690-L1697

I'd be surprised if CTU could be taught to understand that, as it requires understanding our threading model.

@martong Thanks for sharing the results! Would you be able re-run the analysis with the following patch applied to see if any warnings remain (ignoring third party libraries)? :)

Note that the diff is against current master and not 0.18.1.

diff --git a/src/crypto/common.h b/src/crypto/common.h
index e7bb020a1..a5d45b4eb 100644
--- a/src/crypto/common.h
+++ b/src/crypto/common.h
@@ -9,6 +9,7 @@
 #include <config/bitcoin-config.h>
 #endif
 
+#include <cassert>
 #include <stdint.h>
 #include <string.h>
 
@@ -63,6 +64,7 @@ uint32_t static inline ReadBE32(const unsigned char* ptr)
 uint64_t static inline ReadBE64(const unsigned char* ptr)
 {
     uint64_t x;
+    assert(ptr);
     memcpy((char*)&x, ptr, 8);
     return be64toh(x);
 }
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f42a26ca3..fb16b96e9 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3715,6 +3715,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
                     bool fGotBlockFromCache = false;
                     {
                         LOCK(cs_most_recent_block);
+                        assert(pBestIndex);
                         if (most_recent_block_hash == pBestIndex->GetBlockHash()) {
                             if (state.fWantsCmpctWitness || !fWitnessesPresentInMostRecentCompactBlock)
                                 connman->PushMessage(pto, msgMaker.Make(nSendFlags, NetMsgType::CMPCTBLOCK, *most_recent_compact_block));
diff --git a/src/netbase.cpp b/src/netbase.cpp
index d1cde8c40..6d1f3a097 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -422,7 +422,6 @@ static std::string Socks5ErrorString(uint8_t err)
  */
 static bool Socks5(const std::string& strDest, int port, const ProxyCredentials *auth, const SOCKET& hSocket)
 {
-    IntrRecvError recvr;
     LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest);
     if (strDest.size() > 255) {
         return error("Hostname too long");
@@ -443,7 +442,7 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials
         return error("Error sending to proxy");
     }
     uint8_t pchRet1[2];
-    if ((recvr = InterruptibleRecv(pchRet1, 2, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) {
+    if (InterruptibleRecv(pchRet1, 2, SOCKS5_RECV_TIMEOUT, hSocket) != IntrRecvError::OK) {
         LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
         return false;
     }
@@ -466,7 +465,7 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials
         }
         LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password);
         uint8_t pchRetA[2];
-        if ((recvr = InterruptibleRecv(pchRetA, 2, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) {
+        if (InterruptibleRecv(pchRetA, 2, SOCKS5_RECV_TIMEOUT, hSocket) != IntrRecvError::OK) {
             return error("Error reading proxy authentication response");
         }
         if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
@@ -491,6 +490,7 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials
         return error("Error sending to proxy");
     }
     uint8_t pchRet2[4];
+    IntrRecvError recvr;
     if ((recvr = InterruptibleRecv(pchRet2, 4, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) {
         if (recvr == IntrRecvError::Timeout) {
             /* If a timeout happens here, this effectively means we timed out while connecting
diff --git a/src/validation.cpp b/src/validation.cpp
index cc7687ae0..31f433202 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4259,6 +4259,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview,
         return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", ::ChainActive().Height() - pindexFailure->nHeight + 1, nGoodTransactions);
 
     // store block count as we move pindex at check level >= 4
+    assert(pindex);
     int block_count = ::ChainActive().Height() - pindex->nHeight;
 
     // check level 4: try reconnecting blocks
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index 2285579cd..d2b9cb240 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -105,6 +105,7 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
         return stats;
 
     // Find beginning of period
+    assert(stats.period != 0);
     const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period));
     stats.elapsed = pindex->nHeight - pindexEndOfPrevPeriod->nHeight;

@practicalswift I have applied your patch on v0.18.1 and it solves some of the warnings. I've uploaded the results into a new run. You can diff the old and the patched results in the gui, here is the direct link of the diff view which shows the fixed warnings. (Actually, I could have uploaded the results of the patched code to the same old run, then we'd seen that the bugs had changed status to solved.)

Bonus: I've made an analysis with the "sensitive" checkers turned on. These checkers are "alpha" checkers, meaning the false positive ratio can be higher.

Anyway, I encourage you guys to experiment with CodeChecker and with CTU Clang static analyzer, both are free and open software. You could try the different checker profiles, you could mark warnings as false positives if you had your own CodeChecker server running. And the false positive markups would be good feedback for us static analyzer devs (also note that assertions can very well guide the analyzer to avoid false assumptions).

@martong I'm very much interested in collaborating! I'll drop you an e-mail :)

Closing for now.