Remove all Clang Static Analyzer warnings (except one) #602

DesWurstes commented at 10:54 AM on March 22, 2019: contributor

Clang Static Analyzer shows 15 warnings. To see only the relevant ones, I've configured it with

./configure --disable-tests  --disable-benchmark --disable-exhaustive-tests --with-asm=no

which removed the warnings related to inline assembly. Here are the 10 warnings (Some of them are duplicates):

<table class="sortable" style="table-layout:automatic"> <thead><tr> <td>Bug Group</td> <td class="sorttable_sorted">Bug Type<span id="sorttable_sortfwdind"> ▾</span></td> <td>File</td> <td>Function/Method</td> <td class="Q">Line</td> <td class="Q">Path Length</td> <td class="sorttable_nosort"></td>  </tr></thead> <tbody> <tr class="bt_dead_store_dead_assignment"><td class="DESC">Dead store</td><td class="DESC">Dead assignment</td><td>ecmult_gen_impl.h</td><td class="DESC">secp256k1_ecmult_gen</td><td class="Q">153</td><td class="Q">1</td><td><a href="report-8697bd.html#EndPath">View Report</a></td>  </tr> <tr class="bt_dead_store_dead_assignment"><td class="DESC">Dead store</td><td class="DESC">Dead assignment</td><td>ecmult_gen_impl.h</td><td class="DESC">secp256k1_ecmult_gen</td><td class="Q">153</td><td class="Q">1</td><td><a href="report-c12838.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>field_5x52_impl.h</td><td class="DESC">secp256k1_fe_to_storage</td><td class="Q">478</td><td class="Q">15</td><td><a href="report-861420.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>ecmult_gen_impl.h</td><td class="DESC">secp256k1_ecmult_gen_context_is_built</td><td class="Q">95</td><td class="Q">5</td><td><a href="report-48a40b.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>ecmult_impl.h</td><td class="DESC">secp256k1_ecmult_context_is_built</td><td class="Q">357</td><td class="Q">5</td><td><a href="report-5b0264.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>ecmult_gen_impl.h</td><td class="DESC">secp256k1_ecmult_gen_context_is_built</td><td class="Q">95</td><td class="Q">5</td><td><a href="report-a8c644.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>util.h</td><td class="DESC">secp256k1_callback_call</td><td class="Q">24</td><td class="Q">7</td><td><a href="report-1779f3.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>field_5x52_impl.h</td><td class="DESC">secp256k1_fe_to_storage</td><td class="Q">478</td><td class="Q">15</td><td><a href="report-bef9e7.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>util.h</td><td class="DESC">secp256k1_callback_call</td><td class="Q">24</td><td class="Q">7</td><td><a href="report-f5f3af.html#EndPath">View Report</a></td>  </tr> <tr class="bt_logic_error_dereference_of_null_pointer"><td class="DESC">Logic error</td><td class="DESC">Dereference of null pointer</td><td>ecmult_impl.h</td><td class="DESC">secp256k1_ecmult_context_is_built</td><td class="Q">357</td><td class="Q">5</td><td><a href="report-a60846.html#EndPath">View Report</a></td>  </tr> </tbody> <tfoot></tfoot></table>

This pull request silences all of those warnings (except the first two duplicate dead-store warnings, which are solved by #485) by sharing our assumptions with the compiler.

Comments welcome. Would you prefer to remove the do{...}while block?

Check if r->n is nonnull 6d65702fe6

practicalswift commented at 2:19 PM on March 22, 2019: contributor

Concept ACK

Increasing the signal to noise in the Clang Static Analyzer output is valuable in itself.

gmaxwell commented at 10:47 PM on March 22, 2019: contributor

Please don't make verify checks add builtin_unreachables into production code.

You could add a define that does so for analysis purposes (-DANALYSIS_ASSUME_VERIFY_FAILURE_UNREACHABLE or something), but we don't generally want them in production code because the compiler is allowed to assume the condition is impossible and miscompile the code so that arbitrarily bad things can happen if the assumption doesn't hold.

We already explicitly go out of our way to avoid this for the non-null function attribute, to give an example-- we don't compile the library with it visible to prevent the compiler from building the code assuming the various arguments will be null.

(That said, if we found a VERIFY_CHECK that unreaching improved performance with, we might consider it worth doing in that case, if the performance improvement justified extra effort to really check the VERIFY condition! :) )

Pass the assumptions to the compiler d4b0f680c3

DesWurstes force-pushed on Mar 23, 2019

DesWurstes commented at 12:47 PM on March 29, 2019: contributor

Is it okay now? Should I add a configure flag for it?

in src/util.h:44 in d4b0f680c3

  37 | @@ -38,8 +38,10 @@ static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback *
  38 |  
  39 |  #if SECP256K1_GNUC_PREREQ(3, 0)
  40 |  #define EXPECT(x,c) __builtin_expect((x),(c))
  41 | +#define ASSUME(x) do { if (!(x)) __builtin_unreachable(); } while(0)
  42 |  #else
  43 |  #define EXPECT(x,c) (x)
  44 | +#define ASSUME(x) (void)(x)

real-or-random commented at 9:09 PM on March 29, 2019:

Can you make this ((void)(x))?

gmaxwell commented at 12:47 AM on March 31, 2019: contributor

No, it's still running builtin-unreach in normal code. ASSUME is defined, so it always uses it. change the assume If(!x){unreac} to an exit(1) and notice that the library no longer works.

gmaxwell commented at 12:01 AM on May 23, 2019: contributor

This PR has gone inactive, and I don't consider it particularly important. If someone wants to continue this, feel free to reopen.

gmaxwell closed this on May 23, 2019