[net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&) #10977

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:valgrind-getnetworkinfo changing 2 files +24 −26
  1. practicalswift commented at 12:10 PM on August 2, 2017: contributor

    When running test_bitcoin under Valgrind I found the following issue:

    $ valgrind src/test/test_bitcoin
    ...
    ==10465== Use of uninitialised value of size 8
    ==10465==    at 0x6D09B61: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D0B1BB: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<unsigned long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D0B36C: std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::do_put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D17699: std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x4CAAD7: operator<< (ostream:171)
    ==10465==    by 0x4CAAD7: formatValue<ServiceFlags> (tinyformat.h:345)
    ==10465==    by 0x4CAAD7: void tinyformat::detail::FormatArg::formatImpl<ServiceFlags>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:523)
    ==10465==    by 0x1924D4: format (tinyformat.h:510)
    ==10465==    by 0x1924D4: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:803)
    ==10465==    by 0x553A55: vformat (tinyformat.h:947)
    ==10465==    by 0x553A55: format<ServiceFlags> (tinyformat.h:957)
    ==10465==    by 0x553A55: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<ServiceFlags>(char const*, ServiceFlags const&) (tinyformat.h:966)
    ==10465==    by 0x54C952: getnetworkinfo(JSONRPCRequest const&) (net.cpp:462)
    ==10465==    by 0x28EDB5: CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (rpc_tests.cpp:31)
    ==10465==    by 0x293947: rpc_tests::rpc_togglenetwork::test_method() (rpc_tests.cpp:88)
    ==10465==    by 0x2950E5: rpc_tests::rpc_togglenetwork_invoker() (rpc_tests.cpp:84)
    ==10465==    by 0x182496: invoke<void (*)()> (callback.hpp:56)
    ==10465==    by 0x182496: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
    ...
    

    The read of the uninitialized variable nLocalServices is triggered by g_connman->GetLocalServices() in getnetworkinfo(const JSONRPCRequest& request) (net.cpp:462):

    UniValue getnetworkinfo(const JSONRPCRequest& request)
    {
    ...
        if(g_connman)
            obj.push_back(Pair("localservices", strprintf("%016x", g_connman->GetLocalServices())));
    ...
    }
    

    The reason for the uninitialized nLocalServices is that CConnman::Start(...) is not called by the tests, and hence the initialization normally performed by CConnman::Start(...) is not done.

    This commit adds a method Init(const Options& connOptions) which is called by both the constructor and CConnman::Start(...). This method initializes nLocalServices and the other relevant values from the supplied Options object.

  2. practicalswift renamed this:
    [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request)
    [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&)
    on Aug 2, 2017
  3. TheBlueMatt commented at 3:42 PM on August 2, 2017: contributor

    Fixes #9278, I suppose?

  4. practicalswift commented at 7:24 PM on August 2, 2017: contributor

    @TheBlueMatt Yes I think it does! @theuni @achow101 @TheBlueMatt Do you have time to review? :-)

  5. in src/net.cpp:2264 in c5b882ba8c outdated
    2273 | -    nReceiveFloodSize = connOptions.nReceiveFloodSize;
    2274 | -
    2275 | -    nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
    2276 | -    nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
    2277 | -
    2278 |      SetBestHeight(connOptions.nBestHeight);
    


    theuni commented at 9:00 PM on August 2, 2017:

    This can be dropped. The function was used rather than just setting the variable because there used to be a lock for it.


    practicalswift commented at 9:05 PM on August 2, 2017:

    @TheBlueMatt Are you referring to the removal of the line SetBestHeight(connOptions.nBestHeight);? :-)


    theuni commented at 9:19 PM on August 2, 2017:

    Yes. nBestHeight is set already in Init() now.


    practicalswift commented at 9:30 PM on August 2, 2017:

    Now removed! :-)

  6. in src/net.cpp:2205 in c5b882ba8c outdated
    2201 | @@ -2202,6 +2202,9 @@ void CConnman::SetNetworkActive(bool active)
    2202 |  
    2203 |  CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : nSeed0(nSeed0In), nSeed1(nSeed1In)
    2204 |  {
    2205 | +    Options connOptions;
    


    theuni commented at 9:03 PM on August 2, 2017:

    Let's move this to the end in case anything in Init() ends up somehow relying on default values.


    practicalswift commented at 9:06 PM on August 2, 2017:

    @theuni Good point! I'll fix it. Thanks for reviewing.


    practicalswift commented at 9:30 PM on August 2, 2017:

    Fixed!

  7. theuni cross-referenced this on Aug 2, 2017 from issue Pass SendCoinsRecipient (208 bytes) by reference by practicalswift
  8. [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request)
    When running test_bitcoin under Valgrind I found the following issue:
    
    ```
    $ valgrind src/test/test_bitcoin
    ...
    ==10465== Use of uninitialised value of size 8
    ==10465==    at 0x6D09B61: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D0B1BB: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<unsigned long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D0B36C: std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::do_put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x6D17699: std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
    ==10465==    by 0x4CAAD7: operator<< (ostream:171)
    ==10465==    by 0x4CAAD7: formatValue<ServiceFlags> (tinyformat.h:345)
    ==10465==    by 0x4CAAD7: void tinyformat::detail::FormatArg::formatImpl<ServiceFlags>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:523)
    ==10465==    by 0x1924D4: format (tinyformat.h:510)
    ==10465==    by 0x1924D4: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:803)
    ==10465==    by 0x553A55: vformat (tinyformat.h:947)
    ==10465==    by 0x553A55: format<ServiceFlags> (tinyformat.h:957)
    ==10465==    by 0x553A55: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<ServiceFlags>(char const*, ServiceFlags const&) (tinyformat.h:966)
    ==10465==    by 0x54C952: getnetworkinfo(JSONRPCRequest const&) (net.cpp:462)
    ==10465==    by 0x28EDB5: CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (rpc_tests.cpp:31)
    ==10465==    by 0x293947: rpc_tests::rpc_togglenetwork::test_method() (rpc_tests.cpp:88)
    ==10465==    by 0x2950E5: rpc_tests::rpc_togglenetwork_invoker() (rpc_tests.cpp:84)
    ==10465==    by 0x182496: invoke<void (*)()> (callback.hpp:56)
    ==10465==    by 0x182496: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
    ...
    ```
    
    The read of the uninitialized variable nLocalServices is triggered by g_connman->GetLocalServices()
    in getnetworkinfo(const JSONRPCRequest& request) (net.cpp:462):
    
    ```c++
    UniValue getnetworkinfo(const JSONRPCRequest& request)
    {
    ...
        if(g_connman)
            obj.push_back(Pair("localservices", strprintf("%016x", g_connman->GetLocalServices())));
    ...
    }
    ```
    
    The reason for the uninitialized nLocalServices is that CConnman::Start(...) is not called
    by the tests, and hence the initialization normally performed by CConnman::Start(...) is
    not done.
    
    This commit adds a method Init(const Options& connOptions) which is called by both the
    constructor and CConnman::Start(...). This method initializes nLocalServices and the other
    relevant values from the supplied Options object.
    11dd29b658
  9. practicalswift force-pushed on Aug 2, 2017
  10. jonasschnelli added the label P2P on Aug 3, 2017
  11. TheBlueMatt commented at 3:44 PM on August 3, 2017: contributor

    utACK 11dd29b658f60e247069a6adb8a0dcb882858858. Can we take this for 15?

  12. ryanofsky commented at 5:19 PM on August 3, 2017: contributor

    utACK 11dd29b658f60e247069a6adb8a0dcb882858858. Seems fine. But I think a simpler and less fragile bugfix would initialize connman members where they are declared, instead of in distant constructor & init functions.

    --- a/src/net.h
    +++ b/src/net.h
    @@ -383,7 +383,7 @@ private:
         std::atomic<NodeId> nLastNodeId;
     
         /** Services this instance offers */
    -    ServiceFlags nLocalServices;
    +    ServiceFlags nLocalServices = NODE_NONE;
     
         /** Services this instance cares ab
    

    Or if you are concerned about consistent initial values:

    -    ServiceFlags nLocalServices;
    +    ServiceFlags nLocalServices = Options().nLocalServices;
    
  13. sdaftuar commented at 5:54 PM on August 3, 2017: member

    utACK

  14. in src/net.cpp:2277 in 11dd29b658
    2273 | -    nReceiveFloodSize = connOptions.nReceiveFloodSize;
    2274 | -
    2275 | -    nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
    2276 | -    nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
    2277 | -
    2278 | -    SetBestHeight(connOptions.nBestHeight);
    


    jtimon commented at 7:09 PM on August 3, 2017:

    Don't you need to still call SetBestHeight ?


    ryanofsky commented at 8:59 PM on August 3, 2017:

    Don't you need to still call SetBestHeight ?

    This is taken care of by nBestHeight = connOptions.nBestHeight; in Init


    promag commented at 9:15 PM on August 3, 2017:

    Below in CConnman::Init there's nBestHeight = connOptions.nBestHeight;, however the order used in std::atomic<>::operator= is std::memory_order_seq_cst instead of std::memory_order_release.

  15. jtimon commented at 7:15 PM on August 3, 2017: contributor

    utACK modulo nit.

  16. laanwj added this to the milestone 0.15.0 on Aug 3, 2017
  17. in src/net.cpp:2255 in 11dd29b658
    2251 | @@ -2254,30 +2252,15 @@ bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<C
    2252 |      return fBound;
    2253 |  }
    2254 |  
    2255 | -bool CConnman::Start(CScheduler& scheduler, Options connOptions)
    2256 | +bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
    


    promag commented at 9:21 PM on August 3, 2017:

    Nit, rename connOptions to just options as found in the header (the diff will be identical).

  18. in src/net.h:150 in 11dd29b658
     145 | @@ -146,9 +146,26 @@ class CConnman
     146 |          std::vector<CSubNet> vWhitelistedRange;
     147 |          std::vector<CService> vBinds, vWhiteBinds;
     148 |      };
     149 | +
     150 | +    void Init(const Options& connOptions) {
    


    promag commented at 9:23 PM on August 3, 2017:

    Is there a preference to keep this in the header?

  19. in src/net.cpp:2216 in 11dd29b658
    2216 | -    nBestHeight = 0;
    2217 | -    clientInterface = NULL;
    2218 |      flagInterruptMsgProc = false;
    2219 | +
    2220 | +    Options connOptions;
    2221 | +    Init(connOptions);
    


    promag commented at 9:25 PM on August 3, 2017:

    Sounds weird having 2 Init() calls. Is this needed?


    promag commented at 9:25 PM on August 3, 2017:

    Init(Options());?

  20. promag commented at 9:45 PM on August 3, 2017: member

    I tend to agree with ryanofsky comment above.

  21. theuni commented at 7:50 PM on August 4, 2017: member

    utACK

  22. laanwj commented at 11:22 AM on August 5, 2017: member

    I'm just going to merge this, as it has ACKs and fixes the purported issue (and the rc1 deadline is getting close). The code can be improved later.

  23. laanwj merged this on Aug 5, 2017
  24. laanwj closed this on Aug 5, 2017

  25. laanwj referenced this in commit 02f4c4a42e on Aug 5, 2017
  26. fanquake cross-referenced this on Aug 9, 2017 from issue test_bitcoin fails valgrind by TheBlueMatt
  27. PastaPastaPasta referenced this in commit 605c673799 on Aug 6, 2019
  28. PastaPastaPasta referenced this in commit c3b7b62b08 on Aug 6, 2019
  29. PastaPastaPasta referenced this in commit bdc273b0f9 on Aug 6, 2019
  30. PastaPastaPasta referenced this in commit bd8e35cf8d on Aug 7, 2019
  31. PastaPastaPasta referenced this in commit f972bdda0e on Aug 8, 2019
  32. PastaPastaPasta referenced this in commit dfd14bf573 on Aug 12, 2019
  33. barrystyle referenced this in commit 427ca55d18 on Jan 22, 2020
  34. practicalswift cross-referenced this on Mar 8, 2020 from issue build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory by practicalswift
  35. practicalswift cross-referenced this on Oct 1, 2020 from issue signet: Fix uninitialized read in validation by MarcoFalke
  36. practicalswift cross-referenced this on Nov 11, 2020 from issue ci: Remove redundant valgrind fuzz task by MarcoFalke
  37. practicalswift deleted the branch on Apr 10, 2021
  38. furszy cross-referenced this on Nov 5, 2021 from issue [net] Connman options encapsulation by furszy
  39. furszy referenced this in commit 97728e5673 on Dec 7, 2021
  40. bitcoin locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC