Skip to content

Commit

Permalink
Merge #10408, #13291, and partial #13163
Browse files Browse the repository at this point in the history 
Summary:
Merge #10408: Net: Improvements to Tor control port parser
49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg)
0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg)
0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg)
d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg)
29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg)
d8e03c0 torcontrol: Improve comments (Jack Grigg)

Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

Backport of Core PR10408
https://github.com/bitcoin/bitcoin/pull/10408/files

Merge #13291: test: Don't include torcontrol.cpp into the test file
97c112d4ca Declare TorReply parsing functions in torcontrol_tests (Ben Woosley)

Pull request description:

  These methods are standalone string parsing methods which were included
  into test via an include of torcontrol.cpp, which is bad practice.

  ~~Splitting them out reveals that they were the only torcontrol.cpp
  methods under test, so the test file is renamed tor_reply_tests.cpp.~~

  Introduced in #10408

Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

Backport of Core PR13291
bitcoin/bitcoin#13291

(partial) Merge #13163: Make it clear which functions that are intended to be translation unit local
c3f34d06be Make it clear which functions that are intended to be translation unit local (practicalswift)

Pull request description:

  Make it clear which functions that are intended to be translation unit local.

  Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently.

Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53

Backport of Core PR13163
bitcoin/bitcoin#13163

Completes T612

Introduces a memory leak fixed by PR10587
bitcoin/bitcoin#10587

Memory leak fix: https://reviews.bitcoinabc.org/D3285

Includes a fix to the locale linter discussed with fabien

D3285 should be landed immediately after this one

Test Plan:
  make check
  test_runner.py
  sudo apt install tor # if not already installed
  sudo etc/init.d/tor status # to check if tor is running
  sudo etc/init.d/tor start # if it is not already running
  ./src/bitcoind -proxy=127.0.0.1:<tor port>
  ./bitcoin-cli getpeerinfo

tor defaults to port 9050.
`getpeerinfo` should be populated.
arc lint should pass with no errors.

Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3283
  • Loading branch information
@laanwj @jonspock
laanwj authored and jonspock committed Jul 7, 2019
1 parent 9309d59 commit 4f51d74
Show file tree
Hide file tree
Showing 7 changed files with 481 additions and 11 deletions.
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Expand Up @@ -111,6 +111,7 @@ BITCOIN_TESTS =\
test/test_bitcoin.h \
test/test_bitcoin_main.cpp \
test/timedata_tests.cpp \
test/torcontrol_tests.cpp \
test/transaction_tests.cpp \
test/txindex_tests.cpp \
test/txvalidationcache_tests.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/catch_tests/CMakeLists.txt
View file
Expand Up @@ -137,6 +137,7 @@ set(UNIT_CTESTS
streams
sync
timedata
torcontrol
transaction
txindex
txvalidationcache
Expand Down
10 changes: 10 additions & 0 deletions src/catch_tests/catch_unit.h
View file
Expand Up @@ -13,5 +13,15 @@

#define BOOST_ERROR(A) std::cout << (A) << "\n";

#define BOOST_FIXTURE_TEST_SUITE(a,b)
#define BOOST_AUTO_TEST_SUITE_END()

#define STR_VALUE(arg) #arg
// this extra level may not be needed
#define XXX_CASE TEST_CASE
#define BOOST_AUTO_TEST_CASE(name) XXX_CASE(STR_VALUE(name))

#define BOOST_TEST_MESSAGE(a) std::cout << a

#define CATCH_CONFIG_MAIN
#include <catch2/catch.hpp>
185 changes: 185 additions & 0 deletions src/catch_tests/torcontrol_tests.cpp
View file
@@ -0,0 +1,185 @@
// Copyright (c) 2017 The Zcash developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
//
#include <torcontrol.h>

#include <test/test_bitcoin.h>

#include "catch_unit.h"

#include <map>
#include <string>
#include <utility>

std::pair<std::string, std::string> SplitTorReplyLine(const std::string &s);
std::map<std::string, std::string> ParseTorReplyMapping(const std::string &s);

BOOST_FIXTURE_TEST_SUITE(torcontrol_tests, BasicTestingSetup)

static void CheckSplitTorReplyLine(std::string input, std::string command, std::string args) {
BOOST_TEST_MESSAGE(std::string("CheckSplitTorReplyLine(") + input + ")");
auto ret = SplitTorReplyLine(input);
BOOST_CHECK_EQUAL(ret.first, command);
BOOST_CHECK_EQUAL(ret.second, args);
}

BOOST_AUTO_TEST_CASE(util_SplitTorReplyLine) {
BasicTestingSetup setup;
// Data we should receive during normal usage
CheckSplitTorReplyLine("PROTOCOLINFO PIVERSION", "PROTOCOLINFO", "PIVERSION");
CheckSplitTorReplyLine("AUTH METHODS=COOKIE,SAFECOOKIE "
"COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"",
"AUTH",
"METHODS=COOKIE,SAFECOOKIE "
"COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"");
CheckSplitTorReplyLine("AUTH METHODS=NULL", "AUTH", "METHODS=NULL");
CheckSplitTorReplyLine("AUTH METHODS=HASHEDPASSWORD", "AUTH", "METHODS=HASHEDPASSWORD");
CheckSplitTorReplyLine("VERSION Tor=\"0.2.9.8 (git-a0df013ea241b026)\"", "VERSION",
"Tor=\"0.2.9.8 (git-a0df013ea241b026)\"");
CheckSplitTorReplyLine("AUTHCHALLENGE SERVERHASH=aaaa SERVERNONCE=bbbb", "AUTHCHALLENGE",
"SERVERHASH=aaaa SERVERNONCE=bbbb");

// Other valid inputs
CheckSplitTorReplyLine("COMMAND", "COMMAND", "");
CheckSplitTorReplyLine("COMMAND SOME ARGS", "COMMAND", "SOME ARGS");

// These inputs are valid because PROTOCOLINFO accepts an OtherLine that is
// just an OptArguments, which enables multiple spaces to be present
// between the command and arguments.
CheckSplitTorReplyLine("COMMAND ARGS", "COMMAND", " ARGS");
CheckSplitTorReplyLine("COMMAND EVEN+more ARGS", "COMMAND", " EVEN+more ARGS");
}

static void CheckParseTorReplyMapping(std::string input, std::map<std::string, std::string> expected) {
BOOST_TEST_MESSAGE(std::string("CheckParseTorReplyMapping(") + input + ")");
auto ret = ParseTorReplyMapping(input);
BOOST_CHECK_EQUAL(ret.size(), expected.size());
auto r_it = ret.begin();
auto e_it = expected.begin();
while (r_it != ret.end() && e_it != expected.end()) {
BOOST_CHECK_EQUAL(r_it->first, e_it->first);
BOOST_CHECK_EQUAL(r_it->second, e_it->second);
r_it++;
e_it++;
}
}

BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) {
BasicTestingSetup setup;
// Data we should receive during normal usage
CheckParseTorReplyMapping("METHODS=COOKIE,SAFECOOKIE "
"COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"",
{
{"METHODS", "COOKIE,SAFECOOKIE"},
{"COOKIEFILE", "/home/x/.tor/control_auth_cookie"},
});
CheckParseTorReplyMapping("METHODS=NULL", {
{"METHODS", "NULL"},
});
CheckParseTorReplyMapping("METHODS=HASHEDPASSWORD", {
{"METHODS", "HASHEDPASSWORD"},
});
CheckParseTorReplyMapping("Tor=\"0.2.9.8 (git-a0df013ea241b026)\"", {
{"Tor", "0.2.9.8 (git-a0df013ea241b026)"},
});
CheckParseTorReplyMapping("SERVERHASH=aaaa SERVERNONCE=bbbb", {
{"SERVERHASH", "aaaa"},
{"SERVERNONCE", "bbbb"},
});
CheckParseTorReplyMapping("ServiceID=exampleonion1234", {
{"ServiceID", "exampleonion1234"},
});
CheckParseTorReplyMapping("PrivateKey=RSA1024:BLOB", {
{"PrivateKey", "RSA1024:BLOB"},
});
CheckParseTorReplyMapping("ClientAuth=bob:BLOB", {
{"ClientAuth", "bob:BLOB"},
});

// Other valid inputs
CheckParseTorReplyMapping("Foo=Bar=Baz Spam=Eggs", {
{"Foo", "Bar=Baz"},
{"Spam", "Eggs"},
});
CheckParseTorReplyMapping("Foo=\"Bar=Baz\"", {
{"Foo", "Bar=Baz"},
});
CheckParseTorReplyMapping("Foo=\"Bar Baz\"", {
{"Foo", "Bar Baz"},
});

// Escapes
CheckParseTorReplyMapping("Foo=\"Bar\\ Baz\"", {
{"Foo", "Bar Baz"},
});
CheckParseTorReplyMapping("Foo=\"Bar\\Baz\"", {
{"Foo", "BarBaz"},
});
CheckParseTorReplyMapping("Foo=\"Bar\\@Baz\"", {
{"Foo", "Bar@Baz"},
});
CheckParseTorReplyMapping("Foo=\"Bar\\\"Baz\" Spam=\"\\\"Eggs\\\"\"", {
{"Foo", "Bar\"Baz"},
{"Spam", "\"Eggs\""},
});
CheckParseTorReplyMapping("Foo=\"Bar\\\\Baz\"", {
{"Foo", "Bar\\Baz"},
});

// C escapes
CheckParseTorReplyMapping("Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" "
"Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\\2222\" Final=Check",
{
{"Foo", "Bar\nBaz\t"},
{"Spam", "\rEggs"},
{"Octals", "\1a\11\17\1"
"881\377\37"
"8\40"
"0\222"
"2"},
{"Final", "Check"},
});
CheckParseTorReplyMapping("Valid=Mapping Escaped=\"Escape\\\\\"", {
{"Valid", "Mapping"},
{"Escaped", "Escape\\"},
});
CheckParseTorReplyMapping("Valid=Mapping Bare=\"Escape\\\"", {});
CheckParseTorReplyMapping("OneOctal=\"OneEnd\\1\" TwoOctal=\"TwoEnd\\11\"", {
{"OneOctal", "OneEnd\1"},
{"TwoOctal", "TwoEnd\11"},
});

// Special handling for null case
// (needed because string comparison reads the null as end-of-string)
BOOST_TEST_MESSAGE(std::string("CheckParseTorReplyMapping(Null=\"\\0\")"));
auto ret = ParseTorReplyMapping("Null=\"\\0\"");
BOOST_CHECK_EQUAL(ret.size(), 1);
auto r_it = ret.begin();
BOOST_CHECK_EQUAL(r_it->first, "Null");
BOOST_CHECK_EQUAL(r_it->second.size(), 1);
BOOST_CHECK_EQUAL(r_it->second[0], '\0');

// A more complex valid grammar. PROTOCOLINFO accepts a VersionLine that
// takes a key=value pair followed by an OptArguments, making this valid.
// Because an OptArguments contains no semantic data, there is no point in
// parsing it.
CheckParseTorReplyMapping("SOME=args,here MORE optional=arguments here", {
{"SOME", "args,here"},
});

// Inputs that are effectively invalid under the target grammar.
// PROTOCOLINFO accepts an OtherLine that is just an OptArguments, which
// would make these inputs valid. However,
// - This parser is never used in that situation, because the
// SplitTorReplyLine parser enables OtherLine to be skipped.
// - Even if these were valid, an OptArguments contains no semantic data,
// so there is no point in parsing it.
CheckParseTorReplyMapping("ARGS", {});
CheckParseTorReplyMapping("MORE ARGS", {});
CheckParseTorReplyMapping("MORE ARGS", {});
CheckParseTorReplyMapping("EVEN more=ARGS", {});
CheckParseTorReplyMapping("EVEN+more ARGS", {});
}

BOOST_AUTO_TEST_SUITE_END()
1 change: 1 addition & 0 deletions src/test/CMakeLists.txt
View file
Expand Up @@ -130,6 +130,7 @@ add_test_to_suite(devault test_devault
test_bitcoin.cpp
test_bitcoin_main.cpp
timedata_tests.cpp
torcontrol_tests.cpp
transaction_tests.cpp
txindex_tests.cpp
txvalidationcache_tests.cpp
Expand Down

0 comments on commit 4f51d74

Please sign in to comment.