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:
0int32_t blockheight;
1if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
while the optimized code looks like:
00x00000000000f97ab <+123>: callq 0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)>
10x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx
20x00000000000f97b4 <+132>: test %ebx,%ebx
30x00000000000f97b6 <+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>
40x00000000000f97bc <+140>: xor $0x1,%al
50x00000000000f97be <+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:
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:
0==30660== Thread 13 b-httpworker.2:
1==30660== Conditional jump or move depends on uninitialised value(s)
2==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)
3==30660== by 0x2041B9: operator() (rest.cpp:670)
4==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)
5==30660== by 0x3EC994: operator() (std_function.h:706)
6==30660== by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55)
7==30660== by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114)
8==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342)
9==30660== by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60)
10==30660== by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95)
11==30660== by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234)
12==30660== by 0x3EDAAA: operator() (thread:243)
13==30660== by 0x3EDAAA: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186)
14==30660== by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
15==30660== by 0x54876DA: start_thread (pthread_create.c:463)
16==30660== by 0x6DC888E: clone (clone.S:95)
17==30660== Uninitialised value was created by a stack allocation
18==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)
19==30660==
20{
21 <insert_a_suppression_name_here>
22 Memcheck:Cond
23 fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
24 fun:operator()
25 fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_
26 fun:operator()
27 fun:_ZN12HTTPWorkItemclEv
28 fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
29 fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
30 fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
31 fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int>
32 fun:_M_invoke<0, 1, 2>
33 fun:operator()
34 fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv
35 obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
36 fun:start_thread
37 fun:clone
38}
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:
This commit just sets blockheight to -1 as a workaround.
This change was originally made in 41d5d651594c6c939add7a58b7e30c97dccdf24a from #18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested #18740 (review) moving to a new PR, since apparently the error’s been seen on travis previously
