libtorrent causes SIGSEGV with Magnet links giving URI longer than 1024 characters

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Use a long enough magnet link (example: 
http://bitsnoop.com/donald-e-knuth-the-art-of-computer-q3574554.html)

What is the expected output? What do you see instead?
qBittorrent which uses libtorrent crashes with SIGSEGV.

What version of the product are you using? On what operating system?
libtorrent 0.16.9, boost 1.53, qBittorrent 3.0.9, Arch Linux 64bit.

Please provide any additional information below.
I can work around the problem by increasing the length of "ret" array in 
libtorrent::make_magnet_uri to 1096, but that is obviously not an actual 
solution.

Original issue reported on code.google.com by madcatxs...@gmail.com on 27 Apr 2013 at 10:42

[GoogleCodeExporter]

Relevant qbittorrent issue with some backtraces: 
https://github.com/qbittorrent/qBittorrent/issues/612#issuecomment-17114119

I don't know why, but the crash doesn't happen on Windows XP...

Original comment by hammered...@gmail.com on 27 Apr 2013 at 11:53

[GoogleCodeExporter]

I cannot reproduce this on Windows (7 x64) either even when I create some 
ridiculously long magnet link. I don't have any development tools installed on 
the Windows box so I can't check what's going on there, but on Linux it's 
perfectly reproducible, all it takes is a magnet link long enough to overflow 
the array.

Original comment by madcatxs...@gmail.com on 27 Apr 2013 at 6:13

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

The intention in that code is to truncate any link longer than 1024 bytes. I 
just added a unit test for this and it works correctly on Mac OS. I'll try it 
on ubuntu as well.

Maybe it would be worth make it properly resize the target string though, to 
support arbitrarily long urls.

Original comment by ar...@rasterbar.com on 27 Apr 2013 at 8:50

[GoogleCodeExporter]

It does in fact fail on linux. I'll take a look.

Original comment by ar...@rasterbar.com on 27 Apr 2013 at 8:59

[GoogleCodeExporter]

I don't now if you read the comment I deleted because I contained an incorrect 
fix, however, the reason it fails is this:
snprintf from Linux glibc used to return -1 when the output was truncated, but 
since glibc 2.1 it returns number of characters that _would_ have been written 
had the array been long enough as is defined by C99 standard. There is no 
snprintf on Windows so libtorrent uses a wrapper that calls _vsnprintf. 
However, all Windows _*nprintf retain the old behavior. When glibc snprintf is 
called in a loop and the strings it operates on are long enough, it will 
eventually run out of space. This leads to "sizeof(ret) - num_chars" being 
negative, but since snprintf assumes that length is of type "size_t", it treats 
it as a very large number which in turn causes the overflow.

I'd suggest using avoiding the char array altogether:

Original comment by madcatxs...@gmail.com on 27 Apr 2013 at 9:03

Attachments:

• fix_magnet_uri.patch

[GoogleCodeExporter]

fixed in [8385]. Thanks for the report!

Original comment by arvid.no...@gmail.com on 27 Apr 2013 at 9:09

• Changed state: Fixed

[GoogleCodeExporter]

thanks for the patch. I'll apply that in master.

Original comment by arvid.no...@gmail.com on 27 Apr 2013 at 9:11

[GoogleCodeExporter]

Oops, I just realized that instead of calling "ret.resize(1024);" it'd be 
better to use "if (ret.length() > 1024) ret.resize(1024);" to prevent 
unnecessary extending of the resulting string with '\0's. I'm sorry about this 
oversight...

Original comment by madcatxs...@gmail.com on 27 Apr 2013 at 10:05

Attachments:

• fix_magnet_uri.patch