fixed potetial deadlock in bandwidth manager. Fixed http-tracker requests that are sensitive to existing arguments in url (avoids duplicates and doesn't replace arguments)

src/bandwidth_manager.cpp

@@ -61,8 +61,6 @@ namespace libtorrent
 	{
 		INVARIANT_CHECK;
 
-		mutex_t::scoped_lock l(m_mutex);
-
 		// make sure this peer isn't already in line
 		// waiting for bandwidth
 #ifndef NDEBUG
@@ -118,8 +116,6 @@ namespace libtorrent
 
 		if (e) return;
 
-		mutex_t::scoped_lock l(m_mutex);
-
 #if defined TORRENT_LOGGING || defined TORRENT_VERBOSE_LOGGING
 //		(*m_ses->m_logger) << "bw expire [" << m_channel << "]\n";
 #endif
@@ -165,17 +161,21 @@ namespace libtorrent
 
 		pt::ptime now(pt::microsec_clock::universal_time());
 
+		mutex_t::scoped_lock l(m_mutex);
+		int limit = m_limit;
+		l.unlock();
+
 		// available bandwidth to hand out
-		int amount = m_limit - m_current_quota;
-		
+		int amount = limit - m_current_quota;
+
 		int bandwidth_block_size_limit = max_bandwidth_block_size;
-		if (m_queue.size() > 3 && bandwidth_block_size_limit / int(m_queue.size()) > m_limit)
-			bandwidth_block_size_limit = std::max(max_bandwidth_block_size / int(m_queue.size() - 2)
+		if (m_queue.size() > 3 && bandwidth_block_size_limit > limit / int(m_queue.size()))
+			bandwidth_block_size_limit = std::max(max_bandwidth_block_size / int(m_queue.size() - 3)
 				, min_bandwidth_block_size);
 
 		while (!m_queue.empty() && amount > 0)
 		{
-			assert(amount == m_limit - m_current_quota);
+			assert(amount == limit - m_current_quota);
 			intrusive_ptr<peer_connection> peer = m_queue.front();
 			m_queue.pop_front();
 

src/http_tracker_connection.cpp

@@ -84,6 +84,22 @@ namespace
 
 using namespace boost::posix_time;
 
+namespace
+{
+	bool url_has_argument(std::string const& url, std::string argument)
+	{
+		size_t i = url.find('?');
+		if (i == std::string::npos) return false;
+
+		argument += '=';
+
+		if (url.compare(i + 1, argument.size(), argument) == 0) return true;
+		argument.insert(0, "&");
+		return url.find(argument, i)
+			!= std::string::npos;
+	}
+}
+
 namespace libtorrent
 {
 	http_parser::http_parser()
@@ -299,57 +315,100 @@ namespace libtorrent
 		// if request-string already contains
 		// some parameters, append an ampersand instead
 		// of a question mark
-		if (request.find('?') != std::string::npos)
+		size_t arguments_start = request.find('?');
+		if (arguments_start != std::string::npos)
 			m_send_buffer += "&";
 		else
 			m_send_buffer += "?";
 
-		m_send_buffer += "info_hash=";
-		m_send_buffer += escape_string(
-			reinterpret_cast<const char*>(req.info_hash.begin()), 20);
+		if (!url_has_argument(request, "info_hash"))
+		{
+			m_send_buffer += "info_hash=";
+			m_send_buffer += escape_string(
+				reinterpret_cast<const char*>(req.info_hash.begin()), 20);
+			m_send_buffer += '&';
+		}
 
 		if (tracker_req().kind == tracker_request::announce_request)
 		{
-			m_send_buffer += "&peer_id=";
-			m_send_buffer += escape_string(
-				reinterpret_cast<const char*>(req.pid.begin()), 20);
-
-			m_send_buffer += "&port=";
-			m_send_buffer += boost::lexical_cast<std::string>(req.listen_port);
-
-			m_send_buffer += "&uploaded=";
-			m_send_buffer += boost::lexical_cast<std::string>(req.uploaded);
-
-			m_send_buffer += "&downloaded=";
-			m_send_buffer += boost::lexical_cast<std::string>(req.downloaded);
-
-			m_send_buffer += "&left=";
-			m_send_buffer += boost::lexical_cast<std::string>(req.left);
-
-			if (req.web_downloaded > 0)
+			if (!url_has_argument(request, "peer_id"))
 			{
-				m_send_buffer += "&http_downloaded=";
-				m_send_buffer += boost::lexical_cast<std::string>(req.web_downloaded);
+				m_send_buffer += "peer_id=";
+				m_send_buffer += escape_string(
+					reinterpret_cast<const char*>(req.pid.begin()), 20);
+				m_send_buffer += '&';
+			}
+
+			if (!url_has_argument(request, "port"))
+			{
+				m_send_buffer += "port=";
+				m_send_buffer += boost::lexical_cast<std::string>(req.listen_port);
+				m_send_buffer += '&';
+			}
+
+			if (!url_has_argument(request, "uploaded"))
+			{
+				m_send_buffer += "uploaded=";
+				m_send_buffer += boost::lexical_cast<std::string>(req.uploaded);
+				m_send_buffer += '&';
+			}
+
+			if (!url_has_argument(request, "downloaded"))
+			{
+				m_send_buffer += "downloaded=";
+				m_send_buffer += boost::lexical_cast<std::string>(req.downloaded);
+				m_send_buffer += '&';
+			}
+
+			if (!url_has_argument(request, "left"))
+			{
+				m_send_buffer += "left=";
+				m_send_buffer += boost::lexical_cast<std::string>(req.left);
+				m_send_buffer += '&';
 			}
 
 			if (req.event != tracker_request::none)
 			{
-				const char* event_string[] = {"completed", "started", "stopped"};
-				m_send_buffer += "&event=";
-				m_send_buffer += event_string[req.event - 1];
+				if (!url_has_argument(request, "event"))
+				{
+					const char* event_string[] = {"completed", "started", "stopped"};
+					m_send_buffer += "event=";
+					m_send_buffer += event_string[req.event - 1];
+					m_send_buffer += '&';
+				}
+			}
+			if (!url_has_argument(request, "key"))
+			{
+				m_send_buffer += "key=";
+				std::stringstream key_string;
+				key_string << std::hex << req.key;
+				m_send_buffer += key_string.str();
+				m_send_buffer += '&';
+			}
+
+			if (!url_has_argument(request, "compact"))
+			{
+				m_send_buffer += "compact=1&";
+			}
+			if (!url_has_argument(request, "numwant"))
+			{
+				m_send_buffer += "&numwant=";
+				m_send_buffer += boost::lexical_cast<std::string>(
+					std::min(req.num_want, 999));
+				m_send_buffer += '&';
 			}
-			m_send_buffer += "&key=";
-			std::stringstream key_string;
-			key_string << std::hex << req.key;
-			m_send_buffer += key_string.str();
-			m_send_buffer += "&compact=1";
-			m_send_buffer += "&numwant=";
-			m_send_buffer += boost::lexical_cast<std::string>(
-				std::min(req.num_want, 999));
 
 			// extension that tells the tracker that
 			// we don't need any peer_id's in the response
-			m_send_buffer += "&no_peer_id=1";
+			if (!url_has_argument(request, "no_peer_id"))
+			{
+				m_send_buffer += "no_peer_id=1";
+			}
+			else
+			{
+				// remove the trailing '&'
+				m_send_buffer.resize(m_send_buffer.size() - 1);
+			}
 		}
 
 		m_send_buffer += " HTTP/1.0\r\nAccept-Encoding: gzip\r\n"
@@ -588,11 +647,8 @@ namespace libtorrent
 #endif
 			if (has_requester()) requester().tracker_warning("Redirecting to \"" + location + "\"");
 			tracker_request req = tracker_req();
-			std::string::size_type i = location.find('?');
-			if (i == std::string::npos)
-				req.url = location;
-			else
-				req.url.assign(location.begin(), location.begin() + i);
+
+			req.url = location;
 
 			m_man.queue_request(m_strand, req
 				, m_password, m_requester);

src/peer_connection.cpp

@@ -1700,6 +1700,8 @@ namespace libtorrent
 
 	void peer_connection::assign_bandwidth(int channel, int amount)
 	{
+		session_impl::mutex_t::scoped_lock l(m_ses.m_mutex);
+
 #ifdef TORRENT_VERBOSE_LOGGING
 		(*m_logger) << "bandwidth [ " << channel << " ] + " << amount << "\n";
 #endif
@@ -1719,6 +1721,8 @@ namespace libtorrent
 
 	void peer_connection::expire_bandwidth(int channel, int amount)
 	{
+		session_impl::mutex_t::scoped_lock l(m_ses.m_mutex);
+
 		m_bandwidth_limit[channel].expire(amount);
 		if (channel == upload_channel)
 		{

src/torrent.cpp

@@ -1791,6 +1791,8 @@ namespace libtorrent
 
 	void torrent::expire_bandwidth(int channel, int amount)
 	{
+		session_impl::mutex_t::scoped_lock l(m_ses.m_mutex);
+
 		assert(amount >= -1);
 		if (amount == -1) amount = max_bandwidth_block_size;
 		m_bandwidth_limit[channel].expire(amount);
@@ -1810,6 +1812,8 @@ namespace libtorrent
 
 	void torrent::assign_bandwidth(int channel, int amount)
 	{
+		session_impl::mutex_t::scoped_lock l(m_ses.m_mutex);
+
 		assert(amount >= 0);
 		if (amount < max_bandwidth_block_size)
 			expire_bandwidth(channel, max_bandwidth_block_size - amount);