arvidn / libtorrent

an efficient feature complete C++ bittorrent implementation
http://libtorrent.org
Other
5.23k stars 992 forks source link

`add_torrent` in Python does not throw #4538

Closed beroal closed 4 years ago

beroal commented 4 years ago

libtorrent version (or branch): 1.2.5

platform/architecture: Linux beroal 5.6.3-arch1-1 #1 SMP PREEMPT Wed, 08 Apr 2020 07:47:16 +0000 x86_64 GNU/Linux

In the program below, if ti and resume_data do not match, add_torrent should throw an exception according to the documentation. Instead, it terminates the process with a segmentation fault.

import libtorrent
import io
session = libtorrent.session()
torrent_info = libtorrent.torrent_info(libtorrent.bdecode(io.open('firefox.torrent', 'rb').read()))
th = session.add_torrent({'ti': torrent_info, 'save_path': './', 'resume_data': io.open('bt2pd.resume', 'rb').read()})

I see that the error handling code in add_torrent in "libtorrent/bindings/python/src/session.cpp" differs (it throws libtorrent_exception) from one in other functions.

arvidn commented 4 years ago

I don't get a segfault, I get this:

Traceback (most recent call last):
  File "test.py", line 93, in test_add_torrent_error
    self.assertRaises(self.ses.add_torrent({'ti': self.ti, 'save_path': os.getcwd(), 'info_hash': b'abababababababababab'}))
RuntimeError: mismatching info-hash

But that assert still fail, so it doesn't seem to turn into a python exception still

arvidn commented 4 years ago

actually, I wasn't doing that right on the python side. It seems to work for me. let's see what the CI says. https://github.com/arvidn/libtorrent/pull/4554

beroal commented 4 years ago

forgot to attach the files bt2pd.resume.docx firefox.torrent.docx

arvidn commented 4 years ago

I can still not reproduce this. I'm testing on Ubuntu 19.10.

$ cat add.py
import libtorrent
import io
import time
session = libtorrent.session()
torrent_info = libtorrent.torrent_info(libtorrent.bdecode(io.open('firefox.torrent', 'rb').read()))
th = session.add_torrent({'ti': torrent_info, 'save_path': './', 'resume_data': io.open('bt2pd.resume', 'rb').read()})

time.sleep(10)

building with sanitizer:

bjam -j30 address-sanitizer=norecover stage_dependencies stage_module

running:

$ LD_LIBRARY_PATH=./dependencies/ python add.py
beroal commented 4 years ago

With today's version RC_1_2, there is no segmentation fault. The program just runs, no Python exception, and the torrent is added to the session.

arvidn commented 4 years ago

does the unit test pass? test.py?

arvidn commented 4 years ago

btw, are you building with deprecated functions enabled? passing the resume data as a field of add_torrent_params is deprecated. There's a new read_resume_data() which returns an add_torrent_params, and all resume data is stored in that object.

beroal commented 4 years ago

I don't know how to run tests. I build with the ArchLinux build system. Here is the relevant function:

build() {
  cd "libtorrent"

  mkdir -p "_build" && cd "_build"
  cmake \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX="/usr" \
    -DCMAKE_INSTALL_LIBDIR="lib" \
    -Dpython-bindings=ON \
    -Dboost-python-module-name="python" \
    "../"
  make
}

I did not know that it was deprecated. read_resume_data() is not in the manual.

arvidn commented 4 years ago

I did not know that it was deprecated. read_resume_data() is not in the manual.

Oh, maybe I'm mixing this up with what's in libtorrent master.

here it is: https://libtorrent.org/reference-Core.html#read_resume_data()

beroal commented 4 years ago

I ran bindings/python/test.py . “Ran 55 tests. OK.” I prints a lot of text. Should I attach it?

beroal commented 4 years ago

So, is the manual deprecated now?

arvidn commented 4 years ago

So, is the manual deprecated now?

I don't know what you mean. The link I posted was to the manual.

beroal commented 4 years ago

I mean https://libtorrent.org/manual.html .

arvidn commented 4 years ago

ah, yes. That is very outdated. That's for version 0.16.18. I've since then reorganized the documentation into multiple pages, so when I updated the webserver, I didn't delete old files apparently.

I don't think there are any links to that page, are there?

beroal commented 4 years ago

Google finds that page very good. :-)

beroal commented 4 years ago

I wrote another test program that uses read_resume_data. At the end, the torrent is not in the session

No exception is thrown.

import libtorrent
import io
session = libtorrent.session()
torrent_info = libtorrent.torrent_info(libtorrent.bdecode(io.open('firefox.torrent', 'rb').read()))
add_torrent_params = libtorrent.read_resume_data(io.open('bt2pd.resume', 'rb').read())
add_torrent_params.save_path = './'
print(add_torrent_params.info_hash == torrent_info.info_hash())
add_torrent_params.info_hash = torrent_info.info_hash()
th = session.add_torrent(add_torrent_params)
print([th.name() for th in session.get_torrents()])
beroal commented 4 years ago

the torrent file matching the resume file lamb.torrent.docx

arvidn commented 4 years ago

add_torrent_params.info_hash = torrent_info.info_hash()

This is the reason why add_torrent() succeeds. If you clear the info_hash field there is no evidence left that the resume file was for a different info-hash, and it will be accepted as if it was for the new torrent you're adding

beroal commented 4 years ago

I thought that it would detect other differences like that the number of pieces is wrong.

Ah, it does not know the correct number of pieces for the info-hash.

beroal commented 4 years ago

While the program in the issue report still does not throw an exception, I guess, I can do without exceptions. I can compare info-hashes of the resume data and the torrent file. I'll let you know if I encounter difficulties with read_resume_data.

arvidn commented 4 years ago

it still means there's an issue in the backwards-compatibility logic though. setting the resume_data field is supposed to thrown an exception (unless of course deprecated functions are disabled, in which case the resume_data field is ignored)

arvidn commented 4 years ago

After having had a closer look at this, I don't think your example is supposed to fail to add the torrent, it's just supposed to post a fastresume_rejected_alert