Skip to content

fix src/bdict.cc: dict keys should be sorted before encode#8

Merged
Frederick888 merged 1 commit into
Frederick888:masterfrom
ttys3:dict-sorted-by-keys
Jan 5, 2020
Merged

fix src/bdict.cc: dict keys should be sorted before encode#8
Frederick888 merged 1 commit into
Frederick888:masterfrom
ttys3:dict-sorted-by-keys

Conversation

@ttys3

@ttys3 ttys3 commented Jan 3, 2020

Copy link
Copy Markdown
Contributor

this may fixup issue: #7

Comment thread src/zend_container.h Outdated
extern "C" {
#include "php.h"

//c func borrowed from https://github.com/php/php-src/blob/e08ce4c13db6e9aecd3497cd270b72d06c649bc7/ext/standard/array.c#L245

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a space after // and format this file using clang-format? There is already a .clang-format in the project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. force pushed

@Frederick888

Frederick888 commented Jan 4, 2020

Copy link
Copy Markdown
Owner

Ah, good spot. I actually didn't know the fact that 'there is only a single valid bencoding' and sorting keys of dictionaries is apparently needed for this.

There is a minor formatting issue which I already pointed out above and it'd be nice if you could fix it and squash the commits. Thanks for the contribution.

@ttys3
ttys3 force-pushed the dict-sorted-by-keys branch 2 times, most recently from c56e1c2 to b66d7d5 Compare January 5, 2020 11:53
@ttys3

ttys3 commented Jan 5, 2020

Copy link
Copy Markdown
Contributor Author

Yes, I didn't encounter any problems until one day , some users of Transmission reported to me that the torrent file downloaded can not be used because the private tracker server responsed with : torrent not registered on the server.

then, I did some investigation and found that it is that the php extension we used which generated the wrong bittorrent file.

this invalid bittorrent file actually can used with other torrent clients like qBittorrent or Deluge, because when calculating the info hash value, both qBittorrent and Deluge will respect the key orders in the torrent file. But Transmission client will always re-sort the keys in order, which caused the problem.
And, as a side effect, finally I found the bug in the php-bencode extension and fixed it.

@ttys3
ttys3 force-pushed the dict-sorted-by-keys branch from b66d7d5 to af9d284 Compare January 5, 2020 12:35
@ttys3
ttys3 force-pushed the dict-sorted-by-keys branch from af9d284 to 20d4451 Compare January 5, 2020 12:38
@ttys3

ttys3 commented Jan 5, 2020

Copy link
Copy Markdown
Contributor Author

test added

@Frederick888
Frederick888 merged commit 0effba7 into Frederick888:master Jan 5, 2020
@Frederick888

Copy link
Copy Markdown
Owner

Thank you. I'll release a new version asap.

@Frederick888

Copy link
Copy Markdown
Owner

@ttys3

ttys3 commented Feb 13, 2020

Copy link
Copy Markdown
Contributor Author

thanks for the work. v1.2.5 look really awesome

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.

2 participants