From 05f4ce5b6cb317d5b7b2a990df0292581115a869 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Tue, 30 Jan 2007 17:56:42 +0000 Subject: [PATCH] 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 | 18 ++--- src/http_tracker_connection.cpp | 136 ++++++++++++++++++++++---------- src/peer_connection.cpp | 4 + src/torrent.cpp | 4 + 4 files changed, 113 insertions(+), 49 deletions(-) diff --git a/src/bandwidth_manager.cpp b/src/bandwidth_manager.cpp index 77b0c3de1..5ba382c50 100644 --- a/src/bandwidth_manager.cpp +++ b/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 = m_queue.front(); m_queue.pop_front(); diff --git a/src/http_tracker_connection.cpp b/src/http_tracker_connection.cpp index 9474c6077..6c6b19e0f 100755 --- a/src/http_tracker_connection.cpp +++ b/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(req.info_hash.begin()), 20); + if (!url_has_argument(request, "info_hash")) + { + m_send_buffer += "info_hash="; + m_send_buffer += escape_string( + reinterpret_cast(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(req.pid.begin()), 20); - - m_send_buffer += "&port="; - m_send_buffer += boost::lexical_cast(req.listen_port); - - m_send_buffer += "&uploaded="; - m_send_buffer += boost::lexical_cast(req.uploaded); - - m_send_buffer += "&downloaded="; - m_send_buffer += boost::lexical_cast(req.downloaded); - - m_send_buffer += "&left="; - m_send_buffer += boost::lexical_cast(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(req.web_downloaded); + m_send_buffer += "peer_id="; + m_send_buffer += escape_string( + reinterpret_cast(req.pid.begin()), 20); + m_send_buffer += '&'; + } + + if (!url_has_argument(request, "port")) + { + m_send_buffer += "port="; + m_send_buffer += boost::lexical_cast(req.listen_port); + m_send_buffer += '&'; + } + + if (!url_has_argument(request, "uploaded")) + { + m_send_buffer += "uploaded="; + m_send_buffer += boost::lexical_cast(req.uploaded); + m_send_buffer += '&'; + } + + if (!url_has_argument(request, "downloaded")) + { + m_send_buffer += "downloaded="; + m_send_buffer += boost::lexical_cast(req.downloaded); + m_send_buffer += '&'; + } + + if (!url_has_argument(request, "left")) + { + m_send_buffer += "left="; + m_send_buffer += boost::lexical_cast(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::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::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); diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 9ba93d174..39cb5119f 100755 --- a/src/peer_connection.cpp +++ b/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) { diff --git a/src/torrent.cpp b/src/torrent.cpp index a31f73d32..695c2feca 100755 --- a/src/torrent.cpp +++ b/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);