Merge pull request #303 from arvidn/dht-malformed-errors

correctly check incoming DHT error messages

include/libtorrent/kademlia/rpc_manager.hpp

@@ -123,7 +123,7 @@ private:
 	typedef std::multimap<int, observer_ptr> transactions_t;
 #endif
 	transactions_t m_transactions;
-	
+
 	udp_socket_interface* m_sock;
 	dht_logger* m_log;
 	dht_settings const& m_settings;

src/kademlia/node.cpp

@@ -283,11 +283,22 @@ void node::incoming(msg const& m)
 		case 'e':
 		{
 #ifndef TORRENT_DISABLE_LOGGING
-			bdecode_node err = m.message.dict_find_list("e");
-			if (err && err.list_size() >= 2 && m_observer)
+			if (m_observer)
 			{
-				m_observer->log(dht_logger::node, "INCOMING ERROR: %s"
-					, err.list_string_value_at(1).c_str());
+				bdecode_node err = m.message.dict_find_list("e");
+				if (err && err.list_size() >= 2
+					&& err.list_at(0).type() == bdecode_node::int_t
+					&& err.list_at(1).type() == bdecode_node::string_t
+					&& m_observer)
+				{
+					m_observer->log(dht_logger::node, "INCOMING ERROR: (%" PRId64 ") %s"
+						, err.list_int_value_at(0)
+						, err.list_string_value_at(1).c_str());
+				}
+				else
+				{
+					m_observer->log(dht_logger::node, "INCOMING ERROR (malformed)");
+				}
 			}
 #endif
 			node_id id;
@@ -918,7 +929,7 @@ void node::incoming_request(msg const& m, entry& e)
 		}
 
 		reply["token"] = generate_token(m.addr, msg_keys[0].string_ptr());
-		
+
 		m_counters.inc_stats_counter(counters::dht_get_peers_in);
 
 		sha1_hash info_hash(msg_keys[0].string_ptr());

src/kademlia/rpc_manager.cpp

@@ -308,14 +308,25 @@ bool rpc_manager::incoming(msg const& m, node_id* id)
 		, total_milliseconds(now - o->sent()), print_endpoint(m.addr).c_str());
 #endif
 
-	if (m.message.dict_find_string_value("y") == "e") 
+	if (m.message.dict_find_string_value("y") == "e")
 	{
 		// It's an error.
 #ifndef TORRENT_DISABLE_LOGGING
-		bdecode_node err_ent = m.message.dict_find("e");
-		TORRENT_ASSERT(err_ent);
-		m_log->log(dht_logger::rpc_manager, "reply with error from %s: %s"
-			, print_endpoint(m.addr).c_str(), err_ent.list_string_value_at(1).c_str());
+		bdecode_node err = m.message.dict_find("e");
+		if (err && err.list_size() >= 2
+			&& err.list_at(0).type() == bdecode_node::int_t
+			&& err.list_at(1).type() == bdecode_node::string_t)
+		{
+			m_log->log(dht_logger::rpc_manager, "reply with error from %s: (%" PRId64 ") %s"
+				, print_endpoint(m.addr).c_str()
+				, err.list_int_value_at(0)
+				, err.list_string_value_at(1).c_str());
+		}
+		else
+		{
+			m_log->log(dht_logger::rpc_manager, "reply with (malformed) error from %s"
+				, print_endpoint(m.addr).c_str());
+		}
 #endif
 		// Logically, we should call o->reply(m) since we get a reply.
 		// a reply could be "response" or "error", here the reply is an "error".

src/session_impl.cpp

@@ -7069,7 +7069,6 @@ retry:
 		{
 			va_list v;
 			va_start(v, fmt);
-
 			char usr[1024];
 			vsnprintf(usr, sizeof(usr), fmt, v);
 			va_end(v);

test/test_dht.cpp

@@ -48,6 +48,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/kademlia/dht_observer.hpp"
 #include "libtorrent/ed25519.hpp"
 #include <numeric>
+#include <cstdarg>
 
 #include "test.hpp"
 #include "setup_transfer.hpp"
@@ -99,6 +100,8 @@ struct mock_socket : udp_socket_interface
 	bool has_quota() { return true; }
 	bool send_packet(entry& msg, udp::endpoint const& ep, int flags)
 	{
+		// TODO: ideally the mock_socket would contain this queue of packets, to
+		// make tests independent
 		g_sent_packets.push_back(std::make_pair(ep, msg));
 		return true;
 	}
@@ -274,10 +277,6 @@ void send_dht_response(node& node, bdecode_node const& request, udp::endpoint co
 	e["r"].dict().insert(std::make_pair("id", generate_next().to_string()));
 	char msg_buf[1500];
 	int size = bencode(msg_buf, e);
-#if defined TORRENT_DEBUG && TORRENT_USE_IOSTREAM
-// this yields a lot of output. too much
-//	std::cerr << "sending: " <<  e << "\n";
-#endif
 
 #ifdef TORRENT_USE_VALGRIND
 	VALGRIND_CHECK_MEM_IS_DEFINED(msg_buf, size);
@@ -489,11 +488,21 @@ struct obs : dht::dht_observer
 	virtual void outgoing_get_peers(sha1_hash const& target
 		, sha1_hash const& sent_target, udp::endpoint const& ep) TORRENT_OVERRIDE {}
 	virtual void announce(sha1_hash const& ih, address const& addr, int port) TORRENT_OVERRIDE {}
-	virtual void log(dht_logger::module_t l, char const* fmt, ...) TORRENT_OVERRIDE {}
+	virtual void log(dht_logger::module_t l, char const* fmt, ...) TORRENT_OVERRIDE
+	{
+		va_list v;
+		va_start(v, fmt);
+		char buf[1024];
+		vsnprintf(buf, sizeof(buf), fmt, v);
+		va_end(v);
+		m_log.push_back(buf);
+	}
 	virtual void log_packet(message_direction_t dir, char const* pkt, int len
 		, udp::endpoint node) TORRENT_OVERRIDE {}
 	virtual bool on_dht_request(char const* query, int query_len
 		, dht::msg const& request, entry& response) TORRENT_OVERRIDE { return false; }
+
+	std::vector<std::string> m_log;
 };
 
 dht_settings test_settings()
@@ -762,7 +771,7 @@ TORRENT_TEST(dht)
 		fprintf(stderr, "msg: %s\n", print_entry(response).c_str());
 		fprintf(stderr, "   invalid error response: %s\n", error_string);
 	}
-	
+
 	// a node with invalid node-id shouldn't be added to routing table.
 	TEST_EQUAL(node.size().get<0>(), nodes_num);
 
@@ -2280,7 +2289,7 @@ TORRENT_TEST(read_only_node)
 	mock_socket s;
 	obs observer;
 	counters cnt;
-    
+
 	dht::node node(&s, sett, node_id(0), &observer, cnt);
 	udp::endpoint source(address::from_string("10.0.0.1"), 20);
 	bdecode_node response;
@@ -2361,5 +2370,42 @@ TORRENT_TEST(read_only_node)
 	TEST_CHECK(!parsed[3]);
 }
 
+TORRENT_TEST(invalid_error_msg)
+{
+	dht_settings sett = test_settings();
+	mock_socket s;
+	obs observer;
+	counters cnt;
+
+	dht::node node(&s, sett, node_id(0), &observer, cnt);
+	udp::endpoint source(address::from_string("10.0.0.1"), 20);
+
+	entry e;
+	e["y"] = "e";
+	e["e"].string() = "Malformed Error";
+	char msg_buf[1500];
+	int size = bencode(msg_buf, e);
+
+	bdecode_node decoded;
+	error_code ec;
+	bdecode(msg_buf, msg_buf + size, decoded, ec);
+	if (ec) fprintf(stderr, "bdecode failed: %s\n", ec.message().c_str());
+
+	dht::msg m(decoded, source);
+	node.incoming(m);
+
+	bool found = false;
+	for (int i = 0; i < int(observer.m_log.size()); ++i)
+	{
+		if (observer.m_log[i].find("INCOMING ERROR")
+			&& observer.m_log[i].find("(malformed)"))
+			found = true;
+
+		printf("%s\n", observer.m_log[i].c_str());
+	}
+
+	TEST_EQUAL(found, false);
+}
+
 #endif