[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:

     0$ valgrind src/test/test_bitcoin
 1...
 2==10465== Use of uninitialised value of size 8
 3==10465==    at 0x6D09B61: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
 4==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)
 5==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)
 6==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)
 7==10465==    by 0x4CAAD7: operator<< (ostream:171)
 8==10465==    by 0x4CAAD7: formatValue<ServiceFlags> (tinyformat.h:345)
 9==10465==    by 0x4CAAD7: void tinyformat::detail::FormatArg::formatImpl<ServiceFlags>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:523)
10==10465==    by 0x1924D4: format (tinyformat.h:510)
11==10465==    by 0x1924D4: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:803)
12==10465==    by 0x553A55: vformat (tinyformat.h:947)
13==10465==    by 0x553A55: format<ServiceFlags> (tinyformat.h:957)
14==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)
15==10465==    by 0x54C952: getnetworkinfo(JSONRPCRequest const&) (net.cpp:462)
16==10465==    by 0x28EDB5: CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (rpc_tests.cpp:31)
17==10465==    by 0x293947: rpc_tests::rpc_togglenetwork::test_method() (rpc_tests.cpp:88)
18==10465==    by 0x2950E5: rpc_tests::rpc_togglenetwork_invoker() (rpc_tests.cpp:84)
19==10465==    by 0x182496: invoke<void (*)()> (callback.hpp:56)
20==10465==    by 0x182496: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
21...

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

    0UniValue getnetworkinfo(const JSONRPCRequest& request)
1{
2...
3    if(g_connman)
4        obj.push_back(Pair("localservices", strprintf("%016x", g_connman->GetLocalServices())));
5...
6}

    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: member
    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. [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
  8. practicalswift force-pushed on Aug 2, 2017
  9. jonasschnelli added the label P2P on Aug 3, 2017
  10. TheBlueMatt commented at 3:44 pm on August 3, 2017: member
    utACK 11dd29b658f60e247069a6adb8a0dcb882858858. Can we take this for 15?
  11. ryanofsky commented at 5:19 pm on August 3, 2017: member

    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.

    0--- a/src/net.h
1+++ b/src/net.h
2@@ -383,7 +383,7 @@ private:
3     std::atomic<NodeId> nLastNodeId;
4 
5     /** Services this instance offers */
6-    ServiceFlags nLocalServices;
7+    ServiceFlags nLocalServices = NODE_NONE;
8 
9     /** Services this instance cares ab

    Or if you are concerned about consistent initial values:

    0-    ServiceFlags nLocalServices;
1+    ServiceFlags nLocalServices = Options().nLocalServices;
  12. sdaftuar commented at 5:54 pm on August 3, 2017: member
    utACK
  13. 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.
  14. jtimon commented at 7:15 pm on August 3, 2017: contributor
    utACK modulo nit.
  15. laanwj added this to the milestone 0.15.0 on Aug 3, 2017
  16. 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).
  17. 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?
  18. 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());?
  19. promag commented at 9:45 pm on August 3, 2017: member
    I tend to agree with ryanofsky comment above.
  20. theuni commented at 7:50 pm on August 4, 2017: member
    utACK
  21. 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.
  22. laanwj merged this on Aug 5, 2017
  23. laanwj closed this on Aug 5, 2017
  24. laanwj referenced this in commit 02f4c4a42e on Aug 5, 2017
  25. PastaPastaPasta referenced this in commit 605c673799 on Aug 6, 2019
  26. PastaPastaPasta referenced this in commit c3b7b62b08 on Aug 6, 2019
  27. PastaPastaPasta referenced this in commit bdc273b0f9 on Aug 6, 2019
  28. PastaPastaPasta referenced this in commit bd8e35cf8d on Aug 7, 2019
  29. PastaPastaPasta referenced this in commit f972bdda0e on Aug 8, 2019
  30. PastaPastaPasta referenced this in commit dfd14bf573 on Aug 12, 2019
  31. barrystyle referenced this in commit 427ca55d18 on Jan 22, 2020
  32. practicalswift deleted the branch on Apr 10, 2021
  33. furszy referenced this in commit 97728e5673 on Dec 7, 2021
  34. DrahtBot locked this on Aug 16, 2022


practicalswift TheBlueMatt theuni ryanofsky sdaftuar jtimon promag laanwj

Labels
P2P

Milestone
0.15.0

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: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me