maxmustermann/dogecoin
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Merge #18785: Prevent valgrind false positive in rest_blockhash_by_height

Browse source 
fcb7261625 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:

  eef90c14ed/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:🧵:_State_impl<std:🧵:_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 -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 https://github.com/bitcoin/bitcoin/pull/18740#discussion_r414772851 moving to a new PR, since apparently the error's been seen on travis previously

ACKs for top commit:
  MarcoFalke:
    ACK fcb7261625
  practicalswift:
    ACK fcb7261625

Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f
This commit is contained in:
MarcoFalke 2020-04-29 08:23:02 -04:00
parent 0ef0d33f75 fcb7261625
commit ecca2ea1d5
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
1 changed files with 1 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/rest.cpp
View file

@ -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));
}