Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix honoring system.file.allocate.set=1 rtorrent config setting #109

Closed
wants to merge 48 commits into from

Conversation

chros73
Copy link

@chros73 chros73 commented Aug 23, 2016

Fix honoring system.file.allocate.set=1 rtorrent config setting:

  • allocate space for those files only that don't have priority set to off.

It's useful with the following watch directory entry that only loads (in closed state) but not starts torrents, so priority of files can be set.
schedule2 = watch_dir_5, 7, 10, "load.normal=/path/to/watch-dir/load_but_not_start/*.torrent"

Fixes: rakshasa/rtorrent#488

rakshasa and others added 17 commits October 22, 2016 11:22
* Travis improvements.
Currently when a connection triggers a failed_transaction it gets
deleted, which is the expected behavior. Unfortunately any packets
left in the DHT packet priority queue will now have been free'd
leading to a scenario where the process_queue is working on memory
which may contain unpredictable data.

The new behavior proposed by this patch is to also drop any queued
packets to prevent future processing since we're throwing out that
object for probelmatic behavior already.

Notably this seems to reflect the same behavior seen in issue rakshasa#68
however I do not believe that to be a complete fix. It seems to
decrease the probability of the issue occurring. I personally
believe this fix should replace the one applied after rakshasa#68 but am
certainly open to further discussion on the matter.

--------

READ of size 4 thread 0 (rtorrent main)

0. rak::socket_address_inet::address_n() const ../../rak/socket_address.h:167
1. torrent::DhtTransaction::key(rak::socket_address const*, int) (/usr/lib64/libtorrent.so.19+0x2d9936)
2. torrent::DhtTransaction::key(int) const (/usr/lib64/libtorrent.so.19+0x2d8d3f)
3. torrent::DhtServer::process_queue(std::deque<torrent::DhtTransactionPacket*, std::allocator<torrent::DhtTransactionPacket*> >&, unsigned int*) libtorrent/src/dht/dht_server.cc:801
4. torrent::DhtServer::event_write() libtorrent/src/dht/dht_server.cc:866
5. torrent::PollEPoll::perform() libtorrent/src/torrent/poll_epoll.cc:190
6. torrent::PollEPoll::do_poll(long, int) libtorrent/src/torrent/poll_epoll.cc:219
7. torrent::thread_base::event_loop(torrent::thread_base*) libtorrent/src/torrent/utils/thread_base.cc:174
8. main rtorrent/src/main.cc:867
9. __libc_start_main (/lib64/libc.so.6+0x20733)
10. _start (/usr/bin/rtorrent+0x9bce8)

freed by thread 0 (rtorrent main) here:

0. operator delete(void*) (/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/libasan.so.1+0x62e47)
1. torrent::DhtTransactionPing::~DhtTransactionPing() libtorrent/src/dht/dht_transaction.h:359
2. torrent::DhtServer::failed_transaction(std::_Rb_tree_iterator<std::pair<unsigned long const, torrent::DhtTransaction*> >, bool) libtorrent/src/dht/dht_server.cc:672
3. torrent::DhtServer::receive_timeout() libtorrent/src/dht/dht_server.cc:899
4. std::tr1::_Mem_fn<void (torrent::DhtServer::*)()>::operator()(torrent::DhtServer*) const /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:585
5. std::tr1::result_of<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (std::tr1::result_of<std::tr1::_Mu<torrent::DhtServer*, false, false> (torrent::DhtServer*,
std::tr1::tuple<>)>::type)>::type std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)>::__call<, 0>(std::tr1::tuple<> const&,
std::tr1::_Index_tuple<0>) /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:1178
6. std::tr1::result_of<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (std::tr1::result_of<std::tr1::_Mu<torrent::DhtServer*, false, false> (torrent::DhtServer*,
std::tr1::tuple<>)>::type)>::type std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)>::operator()<>()
(/usr/lib64/libtorrent.so.19+0x2e1e5d)
7. std::tr1::_Function_handler<void (), std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)> >::_M_invoke(std::tr1::_Any_data const&) /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:1796
8. std::tr1::function<void ()>::operator()() const (/usr/bin/rtorrent+0xa7b7b)
9. torrent::thread_main::call_events() libtorrent/src/thread_main.cc:82
10. torrent::thread_base::event_loop(torrent::thread_base*) libtorrent/src/torrent/utils/thread_base.cc:141
11. main rtorrent/src/main.cc:867
12. __libc_start_main (/lib64/libc.so.6+0x20733)
13. _start (/usr/bin/rtorrent+0x9bce8)
The macro only gets defined if C++11 is not required, however it
is required. Instead check if we're using c++0x as that macro will
be defined when using --enable-c++0x
Could you add this to rtorrent too?
* Fixed 129, checking peers6 with has_key_string

* Fix for 123, setting limit on Transaction ID for DHT

* Fix for 126 handling resizing exception

* Fix for 125 throw if peer sent metadata_size 0

* Fix for 124 don't throw internal error in bytes_left if left > 1<<60

* Fix for 130, changed order of check in object_read_bencode_c

* Added <stdexcept> for clang

* Revert "Fix for 124 don't throw internal error in bytes_left if left > 1<<60"

This reverts commit 592568e.

* Restrict Transaction ID in DHT. Check length in create_error as well

* Set upper bound of metadata size in DownloadMain::set_metadata_size

* missed () in dht_server

* Handle length check for raw_bencode and raw_string

* only add tid in create_error if it's a raw string
This is a continuation of 856d9ca

I've observed at least two more behaviours where this deletion of
the DhtTransaction within dht_server.cc can later lead to a use
after free when attempting to process the associated packets.

As such this commit updates all of the areas where we're deleting
DhtTransactions to also drop the corresponding packets. While this
arguably could be placed within the DhtTransactions' destructor, it
feels like that would be messing up the hierarchy within the Dht*
classes.
Currently the BEP7 code in tracker_http supports the case where
a peer is IPv6 ready but the tracker is not, passing its IPv6 IP
as an announce parameter. Unfortunately it does not yet support
the case where both the peer and tracker are running in IPv6 mode
but other peers in the swarm are not yet IPv6 capable. This sort
of situation is occuring with increasing frequency now that IPv6
adoption is taking off.

To correct this, we are now going to send the IPv4 parameter to
the tracker when connecting to the tracker over IPv6, implementing
the remaining portion of this specification amendment.
Currently the BEP7 code in tracker_http supports the case where
a peer is IPv6 ready but the tracker is not, passing its IPv6 IP
as an announce parameter. Unfortunately it does not yet support
the case where both the peer and tracker are running in IPv6 mode
but other peers in the swarm are not yet IPv6 capable. This sort
of situation is occuring with increasing frequency now that IPv6
adoption is taking off.

To correct this, we are now going to send the IPv4 parameter to
the tracker when connecting to the tracker over IPv6, implementing
the remaining portion of this specification amendment.
Fix for regression introduced in d0b7724

While some trackers automatically handle decoding encoded URL attributes
others (primarily for performance reasons) stick very close to the spec.

When implementing the &ipv4= parameter I mistakenly added encoding for
symbols to IPv4 when BEP7 only specified that that should be the case
for IPv6. IPv4 is meant to be sent without encoding according to BEP7
and other client behaviour.
@chros73
Copy link
Author

chros73 commented Mar 6, 2017

This one really bothers me.
Actually I started to work on this issue for about 2 months ago (again) and this one triggered the other 4 :)

I completely understand why you didn't want to merge this in its current state, @rakshasa .
Let's modify it that it should never be able to crash the client:

  • not when a download is started (this part is finished)
  • not when priority if files are changed

The latter gives me major headache :)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fa56007 on chros73:fix_honoring_file_allocate_1 into ** on rakshasa:master**.

@chros73
Copy link
Author

chros73 commented May 15, 2017

Omg, what happened! :) I'll create a clean pull request for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

system.file.allocate.set=1 doesn't allocate space for all the files at start
5 participants