Prevent valgrind false positive in rest_blockhash_by_height

A bad interaction between valgrind and clang 6.0.0-1ubuntu2 with -O2
optimizations makes valgrind misleadingly imply C++ code is reading an
uninitialized blockheight value in rest_blockhash_by_height just because that's
what clang optimized code is doing. The C++ code looks like:

    int32_t blockheight;
    if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {

while the optimized code looks like:

    0x00000000000f97ab <+123>:   callq  0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)>
    0x00000000000f97b0 <+128>:   mov    0xc(%rsp),%ebx
    0x00000000000f97b4 <+132>:   test   %ebx,%ebx
    0x00000000000f97b6 <+134>:   js     0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>
    0x00000000000f97bc <+140>:   xor    $0x1,%al
    0x00000000000f97be <+142>:   jne    0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378>

During the rest_interface.py test:

   self.test_rest_request("/blockhashbyheight/", ret_type=RetType.OBJ, status=400)

when height_str is empty, ParseInt32 returns false and blockheight value is
never assigned. The optimized code reads the uninitialized blockheight value
in 0xc(%rsp) before the checking the ParseInt32 return value in %al, which is
harmless, but triggers the following error from valgrind:

==30660== Thread 13 b-httpworker.2:
==30660== Conditional jump or move depends on uninitialised value(s)
==30660==    at 0x2017B6: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:614)
==30660==    by 0x2041B9: operator() (rest.cpp:670)
==30660==    by 0x2041B9: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:301)
==30660==    by 0x3EC994: operator() (std_function.h:706)
==30660==    by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55)
==30660==    by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114)
==30660==    by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342)
==30660==    by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60)
==30660==    by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95)
==30660==    by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234)
==30660==    by 0x3EDAAA: operator() (thread:243)
==30660==    by 0x3EDAAA: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186)
==30660==    by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==30660==    by 0x54876DA: start_thread (pthread_create.c:463)
==30660==    by 0x6DC888E: clone (clone.S:95)
==30660==  Uninitialised value was created by a stack allocation
==30660==    at 0x20173A: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:608)
==30660==
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
   fun:operator()
   fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_
   fun:operator()
   fun:_ZN12HTTPWorkItemclEv
   fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
   fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
   fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
   fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
   fun:_M_invoke<0, 1, 2>
   fun:operator()
   fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv
   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
   fun:start_thread
   fun:clone
}

This is a known bad interaction between clang and valgrind. The clang optimized
code is correct but valgrind has no way of knowing that accessing the
uninitialized value isn't a problem. Issue has been reported previously:

    https://bugs.llvm.org/show_bug.cgi?id=32604#c4
    https://github.com/Z3Prover/z3/issues/972

This commit just sets blockheight to 0 as a workaround.

1 files changed, 1 insertions, 1 deletions

diff --git a/src/rest.cpp b/src/rest.cpp
index 0629557584..aaaaba4cd0 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -607,7 +607,7 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
     std::string height_str;
     const RetFormat rf = ParseDataFormat(height_str, str_uri_part);
 
-    int32_t blockheight;
+    int32_t blockheight = -1; // Initialization done only to prevent valgrind false positive, see https://github.com/bitcoin/bitcoin/pull/18785
     if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
     }