Prevent valgrind false positive in rest_blockhash_by_height #18785

ryanofsky · 2020-04-27T19:20:45Z

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:

bitcoin/test/functional/interface_rest.py

Line 266 in eef90c1

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:

This commit just sets blockheight to -1 as a workaround.

This change was originally made in 41d5d65 from #18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested #18740 (comment) moving to a new PR, since apparently the error's been seen on travis previously

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 Z3Prover/z3#972 This commit just sets blockheight to 0 as a workaround.

maflcko

ACK. Nice catch. I stared at this code for at least half an hour, not knowing that the code is right to begin with.

maflcko · 2020-04-27T19:30:35Z

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 = 0;


Suggested change

int32_t blockheight = 0;

int32_t blockheight = 0; // Initialization only needed as workaround for -O2 compiler optimization and valgrind interaction, see https://github.com/bitcoin/bitcoin/pull/18785

This will prevent valgrind from detecting real issues, so maybe add a comment with motivation?

re: https://github.com/bitcoin/bitcoin/pull/18785/files#r416090195

This will prevent valgrind from detecting real issues, so maybe add a comment with motivation?

Thanks, added comment. I'm not sure if it is too likely to be helpful to someone reading the code, but I guess it's better to have an explanation for something you wouldn't expect to be necessary.

I also changed init value from 0 to -1 just to make it clear that if height is not parsed it will always trigger the error, regardless of the bool return value, and there's no strange case where this is supposed to return information about block 0

ryanofsky

Updated 86433fd -> fcb7261 (pr/grind.1 -> pr/grind.2, compare) with explanation / less confusing init value

ryanofsky · 2020-04-27T20:02:33Z

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 = 0;


re: https://github.com/bitcoin/bitcoin/pull/18785/files#r416090195

This will prevent valgrind from detecting real issues, so maybe add a comment with motivation?

Thanks, added comment. I'm not sure if it is too likely to be helpful to someone reading the code, but I guess it's better to have an explanation for something you wouldn't expect to be necessary.

I also changed init value from 0 to -1 just to make it clear that if height is not parsed it will always trigger the error, regardless of the bool return value, and there's no strange case where this is supposed to return information about block 0

maflcko · 2020-04-27T20:14:55Z

ACK fcb7261

DrahtBot · 2020-04-27T23:05:11Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove g_rpc_node global #18740 (Remove g_rpc_node global by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2020-04-28T06:29:11Z

ACK fcb7261

Note that MemorySanitizer is not fooled by this type of optimization.

MemorySanitizer can be trivially be enabled in Travis by merging #18288. Review welcome :)

Valgrind has well documented optimization issues. From the Valgrind documentation:

If you are planning to use Memcheck: On rare occasions, compiler optimisations (at -O2 and above, and sometimes -O1) have been observed to generate code which fools Memcheck into wrongly reporting uninitialised value errors, or missing uninitialised value errors. We have looked in detail into fixing this, and unfortunately the result is that doing so would give a further significant slowdown in what is already a slow tool. So the best solution is to turn off optimisation altogether. Since this often makes things unmanageably slow, a reasonable compromise is to use -O. This gets you the majority of the benefits of higher optimisation levels whilst keeping relatively small the chances of false positives or false negatives from Memcheck. Also, you should compile your code with -Wall because it can identify some or all of the problems that Valgrind can miss at the higher optimisation levels. (Using -Wall is also a good idea in general.)

…h_by_height fcb7261 Prevent valgrind false positive in rest_blockhash_by_height (Russell Yanofsky) Pull request description: 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: ```c++ 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: https://github.com/bitcoin/bitcoin/blob/eef90c14ed0f559e3f6e187341009270b84f45cb/test/functional/interface_rest.py#L266 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 - Z3Prover/z3#972 This commit just sets blockheight to -1 as a workaround. This change was originally made in 41d5d65 from bitcoin#18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested bitcoin#18740 (comment) moving to a new PR, since apparently the error's been seen on travis previously ACKs for top commit: MarcoFalke: ACK fcb7261 practicalswift: ACK fcb7261 Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f

This should no-longer be necessary after bitcoin#18785.

d7120f7 valgrind : remove duplicate BCLog::Logger suppression (fanquake) 708e3c7 valgrind: remove rest_blockhash_by_height suppression (fanquake) Pull request description: bitcoin@708e3c7: `Suppress rest_blockhash_by_height` should no longer be needed after bitcoin#18785. bitcoin@d7120f7 : Removes a duplicate `Suppress BCLog::Logger::StartLogging()` suppression that was added in bitcoin#17770. ACKs for top commit: MarcoFalke: ACK d7120f7 practicalswift: ACK d7120f7 -- patch looks correct and valgrind Travis job is happy Tree-SHA512: 45f5b9fa64bf83cada3cd9ad33c245f660376d5b29f51a2531d83133940090df945f5ef26c5847d6ec024ffab9528d55573c5cf9ca5e73795f9abfc971b3d29b

This should no-longer be necessary after bitcoin#18785.

…h_by_height fcb7261 Prevent valgrind false positive in rest_blockhash_by_height (Russell Yanofsky) Pull request description: 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: ```c++ 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: https://github.com/bitcoin/bitcoin/blob/eef90c14ed0f559e3f6e187341009270b84f45cb/test/functional/interface_rest.py#L266 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 - Z3Prover/z3#972 This commit just sets blockheight to -1 as a workaround. This change was originally made in 41d5d65 from bitcoin#18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested bitcoin#18740 (comment) moving to a new PR, since apparently the error's been seen on travis previously ACKs for top commit: MarcoFalke: ACK fcb7261 practicalswift: ACK fcb7261 Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f

maflcko approved these changes Apr 27, 2020

View reviewed changes

ryanofsky mentioned this pull request Apr 27, 2020

Remove g_rpc_node global #18740

Merged

ryanofsky force-pushed the pr/grind branch from 86433fd to fcb7261 Compare April 27, 2020 20:03

ryanofsky commented Apr 27, 2020

View reviewed changes

DrahtBot added the RPC/REST/ZMQ label Apr 27, 2020

practicalswift mentioned this pull request Apr 28, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

maflcko merged commit ecca2ea into bitcoin:master Apr 29, 2020

fanquake added a commit to fanquake/bitcoin that referenced this pull request May 5, 2020

valgrind: remove rest_blockhash_by_height suppression

708e3c7

This should no-longer be necessary after bitcoin#18785.

fanquake mentioned this pull request May 5, 2020

valgrind: remove outdated suppressions #18879

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2020

valgrind: remove rest_blockhash_by_height suppression

07718ed

This should no-longer be necessary after bitcoin#18785.

ryanofsky mentioned this pull request Mar 5, 2021

ScanForWalletTransactions: jump or move depends on uninitialised value #21367

Closed

maflcko mentioned this pull request Aug 4, 2021

Replace boost::optional with std::optional #20671

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent valgrind false positive in rest_blockhash_by_height #18785

Prevent valgrind false positive in rest_blockhash_by_height #18785

Uh oh!

ryanofsky commented Apr 27, 2020 •

edited

Loading

Uh oh!

maflcko left a comment

Uh oh!

maflcko Apr 27, 2020

Uh oh!

ryanofsky Apr 27, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Apr 27, 2020

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

DrahtBot commented Apr 27, 2020

Uh oh!

practicalswift commented Apr 28, 2020

Uh oh!

Uh oh!

	int32_t blockheight = 0;
	int32_t blockheight = 0; // Initialization only needed as workaround for -O2 compiler optimization and valgrind interaction, see https://github.com/bitcoin/bitcoin/pull/18785

Prevent valgrind false positive in rest_blockhash_by_height #18785

Prevent valgrind false positive in rest_blockhash_by_height #18785

Uh oh!

Conversation

ryanofsky commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

DrahtBot commented Apr 27, 2020

Conflicts

Uh oh!

practicalswift commented Apr 28, 2020

Uh oh!

Uh oh!

ryanofsky commented Apr 27, 2020 •

edited

Loading