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

Small fixes to parsing ports #228

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Conversation

0xTylerHolmes
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR address?

This PR addresses some inconsistencies in the parsing of ports in various formats.

  1. No longer allows negative integer port values to be silently converted to uints
  2. Remove incorrect error string for max uint16 values. (Previously the wrong value for a MAX_UINT16 was logged)
  3. Replace AtoI and ItoA strcov calls with ParseUint and FormatUint
  4. Add test for negative udp/tcp ports

Potential Changes

I elected to replace the AtoI and ItoA with the parse/format uints with base16. These require less checks at the cost of less robust errors. If this seems like too big of a trade off I can reformat it back to the old way while still maintaining the bug fixes.

multiaddr_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

That a great fix thx, please update the test so it only test the invalid port (not invalid port and invalid unix component)

add invalid unix component to test

Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM thx for this

@Jorropo Jorropo merged commit ac5c928 into multiformats:master Jan 24, 2024
9 checks passed
@0xTylerHolmes 0xTylerHolmes deleted the port-parsing branch January 28, 2024 14:49
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