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.

Sign up for GitHub

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

Jump to bottom

fix: encode ip in network byte order for udp announce #6126

Merged
merged 2 commits into from Oct 18, 2023
Merged

fix: encode ip in network byte order for udp announce #6126

merged 2 commits into from Oct 18, 2023

Conversation

tearfur
Copy link
Member

@tearfur tearfur commented Oct 17, 2023 •
edited

Fixes #6125.

Notes: Fixed 4.0.0 bug where the IP address field in UDP announces are not encoded in network byte order. [BEP-15]

@tearfur tearfur added this to the 4.1.0 milestone Oct 17, 2023
@ckerr
Copy link
Member

ckerr commented Oct 17, 2023

!!!

ckerr
ckerr reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

👍 for the simple, straightforward fix.

Just brainstorming here... the code smell that sticks out to me that made this bug possible is that announce_ip is in a raw uint32_t type. The reason this doesn't happen to ports is because of the tr_port class that always gets byte ordering right. What if there was a tr_buffer::add_addr(tr_address const&); method similar to tr_buffer::add_port()? (...and if there was, would it actually work for all the places where we write an address?)

@ckerr ckerr modified the milestones: 4.1.0, 4.0.x Oct 17, 2023
@tearfur
Copy link
Member Author

tearfur commented Oct 18, 2023

What if there was a tr_buffer::add_addr(tr_address const&); method similar to tr_buffer::add_port()? (...and if there was, would it actually work for all the places where we write an address?)

Yeah, why not? It should work, otherwise tr_address::to_compact() wouldn't work either.

@ckerr ckerr merged commit c70c49e into transmission:main Oct 18, 2023
21 checks passed
@tearfur tearfur deleted the udp-announce branch October 19, 2023 00:59
@robd003
Copy link
Contributor

robd003 commented Oct 19, 2023

Is it worth releasing an update so all 4.0.x users aren't advertising incorrect addresses to trackers?

@tearfur
Copy link
Member Author

tearfur commented Oct 20, 2023

I think so, this fix warrants another 4.0.x release.

tearfur added a commit to tearfur/transmission that referenced this pull request Oct 20, 2023
@tearfur
fix: encode ip in network byte order for udp announce (transmission…
762932f 
…#6126)

(cherry picked from commit c70c49e)
tearfur added a commit to tearfur/transmission that referenced this pull request Oct 20, 2023
@tearfur
fixup! fix: encode ip in network byte order for udp announce (trans…
f175114 
…mission#6126)
ckerr pushed a commit that referenced this pull request Oct 20, 2023
@tearfur
fix: encode ip in network byte order for udp announce (#6126) (#6132)
90b86a1 
* fix: encode `ip` in network byte order for udp announce (#6126)

(cherry picked from commit c70c49e)

* fixup! fix: encode `ip` in network byte order for udp announce (#6126)
@tearfur tearfur added notes:highlight Should be listed prominently in release notes and removed notes:highlight Should be listed prominently in release notes labels Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

Assignees
No one assigned
Labels
bep-compliance scope:core type:fix A bug fix
Milestone
4.0.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tearfur @ckerr @robd003