From fcb72616253ed22e364bc312992d77efc1c4a3c1 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 24 Apr 2020 13:07:07 -0400 Subject: 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 , std::allocator > const&, int*)> 0x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx 0x00000000000f97b4 <+132>: test %ebx,%ebx 0x00000000000f97b6 <+134>: js 0xf98aa , std::allocator > const&)+378> 0x00000000000f97bc <+140>: xor $0x1,%al 0x00000000000f97be <+142>: jne 0xf98aa , std::allocator > 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, std::allocator > const&) (rest.cpp:614) ==30660== by 0x2041B9: operator() (rest.cpp:670) ==30660== by 0x2041B9: std::_Function_handler, std::allocator > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string, std::allocator > 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::Run() (httpserver.cpp:114) ==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue*, int) (httpserver.cpp:342) ==30660== by 0x3EDAAA: __invoke_impl *, int), WorkQueue *, int> (invoke.h:60) ==30660== by 0x3EDAAA: __invoke *, int), WorkQueue *, 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*, int), WorkQueue*, 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, std::allocator > const&) (rest.cpp:608) ==30660== { 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 *, int), WorkQueue *, int> fun:__invoke *, int), WorkQueue *, 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. --- src/rest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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)); } -- cgit v1.2.3