[net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&) by practicalswift · Pull Request #10977 · bitcoin/bitcoin

practicalswift · 2017-08-02T12:10:16Z

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.

TheBlueMatt · 2017-08-02T15:42:31Z

Fixes #9278, I suppose?

practicalswift · 2017-08-02T19:24:41Z

@TheBlueMatt Yes I think it does!

@theuni @achow101 @TheBlueMatt Do you have time to review? :-)

theuni · 2017-08-02T21:00:06Z

src/net.cpp

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

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

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

Now removed! :-)

theuni · 2017-08-02T21:03:09Z

src/net.cpp

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

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

…equest& 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.

TheBlueMatt · 2017-08-03T15:44:53Z

utACK 11dd29b. Can we take this for 15?

ryanofsky

utACK 11dd29b. 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;

sdaftuar · 2017-08-03T17:54:26Z

utACK

jtimon · 2017-08-03T19:09:32Z

src/net.cpp

-    nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
-    nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
-
-    SetBestHeight(connOptions.nBestHeight);


Don't you need to still call SetBestHeight ?

Don't you need to still call SetBestHeight ?

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

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.

jtimon · 2017-08-03T19:15:31Z

utACK modulo nit.

promag · 2017-08-03T21:15:19Z

src/net.cpp

-    nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
-    nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
-
-    SetBestHeight(connOptions.nBestHeight);


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.

promag · 2017-08-03T21:21:53Z

src/net.cpp

 }

-bool CConnman::Start(CScheduler& scheduler, Options connOptions)
+bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)


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

promag · 2017-08-03T21:23:28Z

src/net.h

        std::vector<CService> vBinds, vWhiteBinds;
    };
+
+    void Init(const Options& connOptions) {


Is there a preference to keep this in the header?

promag · 2017-08-03T21:25:05Z

src/net.cpp

    flagInterruptMsgProc = false;
+
+    Options connOptions;
+    Init(connOptions);


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

Init(Options());?

promag · 2017-08-03T21:45:11Z

I tend to agree with ryanofsky comment above.

theuni · 2017-08-04T19:50:58Z

utACK

laanwj · 2017-08-05T11:22:55Z

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.

…const JSONRPCRequest&) 11dd29b [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request) (practicalswift) Pull request description: 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. Tree-SHA512: d8742363acffd03b2ee081cc56840275569e17edc6fa4bb1dee4a5971ffe4b8ab1d2fe7b68f98a086bf133b7ec46f4e471243ca08b45bf82356e8c831a5a5f21

…rkinfo(const JSONRPCRequest&) 11dd29b [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request) (practicalswift) Pull request description: 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. Tree-SHA512: d8742363acffd03b2ee081cc56840275569e17edc6fa4bb1dee4a5971ffe4b8ab1d2fe7b68f98a086bf133b7ec46f4e471243ca08b45bf82356e8c831a5a5f21

e1d12d3 Add Clang thread safety analysis annotations (furszy) 5716940 net: Add missing locks in net.{cpp,h} (furszy) 8c02b59 net: simplify fRelayTxes flag processing (furszy) 71667df remove unused IsArgSet check (Marko Bencun) 729c63d add m_added_nodes to connman options (Marko Bencun) 8c8ad18 [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request) (practicalswift) a13b7c9 Add vConnect to CConnman::Options (Marko Bencun) 987342e ActiveMasternode: fix not initialized socket. (furszy) 8d788ba add SeedNodes to CConnman::Options (Marko Bencun) d9e91ff add Binds, WhiteBinds to CConnman::Options (Marko Bencun) 41c89af add WhitelistedRange to CConnman::Options (Marko Bencun) Pull request description: More groundwork for the LLMQ sessions connections work, built on top of #2586 and #2587 (starts in 10efb72a). Focused on cleaning the connman init/start by decoupling the command line arguments. Backported PRs list: * bitcoin#10467. * bitcoin#10496. * bitcoin#10596. * bitcoin#10977. * bitcoin#11301. * bitcoin#11744 (partially, without the outbound members changes as we don't have them). ACKs for top commit: random-zebra: utACK e1d12d3 Fuzzbawls: ACK e1d12d3 Tree-SHA512: 81a1ab7a1e7f487330354631ee728be9ec78223fe4978c8b9c97b7fbc8d2bfe4f4ea9e88ac4a3d1f0553f7adad871c81261b1a7545bae710a4e3200b8a5031d7

practicalswift changed the title ~~[net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request)~~ [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&) Aug 2, 2017

theuni reviewed Aug 2, 2017

View reviewed changes

theuni mentioned this pull request Aug 2, 2017

Pass SendCoinsRecipient (208 bytes) by reference #10964

Merged

practicalswift force-pushed the valgrind-getnetworkinfo branch from c5b882b to 11dd29b Compare August 2, 2017 21:28

jonasschnelli added the P2P label Aug 3, 2017

ryanofsky reviewed Aug 3, 2017

View reviewed changes

jtimon reviewed Aug 3, 2017

View reviewed changes

laanwj added this to the 0.15.0 milestone Aug 3, 2017

promag reviewed Aug 3, 2017

View reviewed changes

laanwj merged commit 11dd29b into bitcoin:master Aug 5, 2017

fanquake mentioned this pull request Aug 9, 2017

test_bitcoin fails valgrind #9278

Closed

practicalswift mentioned this pull request Mar 8, 2020

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

Merged

practicalswift mentioned this pull request Oct 1, 2020

signet: Fix uninitialized read in validation #20035

Merged

practicalswift mentioned this pull request Nov 11, 2020

ci: Remove redundant valgrind fuzz task #20368

Merged

practicalswift deleted the valgrind-getnetworkinfo branch April 10, 2021 19:32

furszy mentioned this pull request Nov 5, 2021

[net] Connman options encapsulation PIVX-Project/PIVX#2632

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Aug 2, 2017

Uh oh!

practicalswift commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 3, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 3, 2017

Uh oh!

theuni commented Aug 4, 2017

Uh oh!

laanwj commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

practicalswift commented Aug 2, 2017 •

edited

Loading

practicalswift Aug 2, 2017 •

edited

Loading

laanwj commented Aug 5, 2017 •

edited

Loading