Fix Clang Static Analyzer warnings #12963

practicalswift commented at 6:30 AM on April 12, 2018: contributor

Fix Clang Static Analyzer warnings reported by @kallewoof in #12961:

Fix dead stores. Values were stored but never read.
Add assertion to guide static analyzers. See #12961 for details.

Empact commented at 6:37 AM on April 12, 2018: member

Could you elaborate on why it needs the guidance?

practicalswift commented at 6:39 AM on April 12, 2018: contributor

@Empact See the report linked in #12961 :-)

fanquake added the label Refactoring on Apr 12, 2018

in src/rpc/mining.cpp:528 in 2c6b31802f outdated

 524 | @@ -525,6 +525,7 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
 525 |          // Need to update only after we know CreateNewBlock succeeded
 526 |          pindexPrev = pindexPrevNew;
 527 |      }
 528 | +    assert(pindexPrev && &pblocktemplate->block);

kallewoof commented at 6:45 AM on April 12, 2018:

Are you sure the analyzer doesn't realize a reference to an object is never null? Weird.

practicalswift commented at 6:52 AM on April 12, 2018:

@kallewoof Agreed – it should understand that. Checking pindexPrev should be sufficient. Updated. Please re-review :-)

practicalswift force-pushed on Apr 12, 2018

kallewoof commented at 7:22 AM on April 12, 2018: member

utACK 7e95d9772f3e576e514a73fbade932cf5024c474

Clang Analyzer no longer reports the corresponding issues.

Empact commented at 7:26 AM on April 12, 2018: member

utACK 7e95d97

in src/test/util_tests.cpp:952 in 2c7972f528 outdated

 948 | @@ -949,7 +949,7 @@ static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
 949 |          case UnlockCommand:
 950 |              ReleaseDirectoryLocks();
 951 |              ch = true; // Always succeeds
 952 | -            rv = write(fd, &ch, 1);

ajtowns commented at 9:26 AM on April 12, 2018:

Should this be fixed by adding an assert(rv == 1); instead, matching the LockCommand case?

in src/wallet/test/coinselector_tests.cpp:550 in 2c7972f528 outdated

 546 | @@ -547,7 +547,6 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
 547 |      {
 548 |          // Reset
 549 |          out_value = 0;
 550 | -        target = 0;

ajtowns commented at 9:33 AM on April 12, 2018:

Might make sense to reset out_value and out_set at the sample place that target actually gets defined.

in src/rpc/mining.cpp:528 in 7e95d9772f outdated

 524 | @@ -525,6 +525,7 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
 525 |          // Need to update only after we know CreateNewBlock succeeded
 526 |          pindexPrev = pindexPrevNew;
 527 |      }
 528 | +    assert(pindexPrev);

ajtowns commented at 10:11 AM on April 12, 2018:

I believe the reason this is valid is that we've checked IsInitialBlockDownload() is false, and that ensures chainActive->Tip() is not nullptr, and we've just ensured pindexPrev is chainActive->Tip(). We did potentially release cs_main in the meantime, though. I wonder if asserting chainActive->Tip() != nullptr or !IsInitialBlockDownload() would be enough to satisfy to the static analysis; either seems like it would be easier to understand, to me?

practicalswift commented at 10:36 AM on April 12, 2018:

@ajtowns On what line do you suggest placing that assertion?

ajtowns commented at 12:37 PM on April 12, 2018:

If it satisfied the analysis tool, I'd probably put assert(chainActive->Tip() != nullptr); somewhere around static CBlockIndex* pindexPrev;, I guess.

ajtowns approved

ajtowns commented at 10:13 AM on April 12, 2018: member

utACK 7e95d9772f3e576e514a73fbade932cf5024c474

I had a quick go at trying to get the static analyzer to work but didn't strike it lucky.

practicalswift force-pushed on Apr 12, 2018

practicalswift commented at 12:31 PM on April 12, 2018: contributor

Updated. Please re-review :-)

Fix dead stores. Values were stored but never read. Limit scope. fd447a6efe

Add assertion to guide static analyzers. Clang Static Analyzer needs this guidance. 159c32d1f1

in src/script/standard.h:95 in 5b01fd9a42 outdated

  89 | @@ -90,9 +90,9 @@ struct WitnessV0KeyHash : public uint160
  90 |  //! CTxDestination subtype to encode any future Witness version
  91 |  struct WitnessUnknown
  92 |  {
  93 | -    unsigned int version;
  94 | -    unsigned int length;
  95 | -    unsigned char program[40];
  96 | +    unsigned int version = 0;
  97 | +    unsigned int length = 0;
  98 | +    unsigned char program[40] = {};

ajtowns commented at 1:00 PM on April 12, 2018:

The warning is only reasonable if an uninitialised WitnessUnknown is compared with something; that seems a bug in its own right, and not something that should be hidden by auto initialising everything to 0/empty?

How about:

+    template<typename T>
+    inline WitnessUnknown(unsigned int _version, T _data)
+      : version(_version), length(_data.size())
+    {
+        assert(length <= 40);
+        std::copy(_data.begin(), _data.end(), program);
+    }
+
+    WitnessUnknown() = delete;

and changing the various uses from:

-            WitnessUnknown unk;
-            unk.version = version;
-            std::copy(data.begin(), data.end(), unk.program);
-            unk.length = data.size();
-            return unk;

to just

+            return WitnessUnknown(version, data);

As far as I can see the warning is a false-positive though?

EDIT: oops, markdown formatting missed the template<>

ajtowns commented at 1:08 PM on April 12, 2018:

Thinking about it a bit more, that doesn't guarantee the first length bytes of program are initialised. std::copy_n(_data, length, program); might arguably be better.

practicalswift commented at 6:03 PM on April 12, 2018:

Skipped this change to keep things easy to review. I might post a follow-up PR instead :-)

practicalswift force-pushed on Apr 12, 2018

ajtowns commented at 6:15 AM on April 13, 2018: member

utACK 159c32d1f111e6bad490bd23ae215462e8ba4374

practicalswift commented at 5:46 PM on April 28, 2018: contributor

@kallewoof @Empact Would you mind re-reviewing the latest version? :-)

Empact commented at 3:43 PM on April 29, 2018: member

utACK 159c32d

kallewoof commented at 10:25 AM on May 7, 2018: member

utACK 159c32d1f111e6bad490bd23ae215462e8ba4374

practicalswift commented at 6:15 PM on May 13, 2018: contributor

@MarcoFalke @laanwj Willing to review? :-)

MarcoFalke commented at 6:32 PM on May 13, 2018: member

utACK 159c32d1f111e6bad490bd23ae215462e8ba4374

ken2812221 commented at 12:36 AM on May 14, 2018: contributor

utACK 159c32d1f111e6bad490bd23ae215462e8ba4374

MarcoFalke merged this on May 14, 2018

MarcoFalke closed this on May 14, 2018

MarcoFalke referenced this in commit c5870ab689 on May 14, 2018

practicalswift deleted the branch on Apr 10, 2021

UdjinM6 referenced this in commit 531d8ceade on May 21, 2021

UdjinM6 referenced this in commit 32e84f8ea4 on May 25, 2021

gades referenced this in commit eb8d634463 on Apr 21, 2022

DrahtBot locked this on Aug 16, 2022