ntop: Measure address size in bytes #61
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knu
reviewed
Nov 27, 2023
|
Makes sense. I just though such a change would break compatibility, but actually the affected user codes have never worked correctly. |
knu
reviewed
Nov 28, 2023
hanazuki
force-pushed
the
ntop-bytesize
branch
from
dab4c25
to
d1e85d0
Compare
|
@knu I just updated the patch. Please take a look. |
hanazuki
force-pushed
the
ntop-bytesize
branch
from
d1e85d0
to
88652ff
Compare
knu
reviewed
Nov 28, 2023
| @@ -111,7 +111,11 @@ def self.new_ntoh(addr) | |||
| # Convert a network byte ordered string form of an IP address into | |||
| # human readable form. | |||
There was a problem hiding this comment.
Could you add something like this here? "It expects a string encoded in Encoding::ASCII_8BIT."
hanazuki
force-pushed
the
ntop-bytesize
branch
from
88652ff
to
be01430
Compare
knu
reviewed
Nov 28, 2023
lib/ipaddr.rb
Outdated
| @@ -110,8 +110,13 @@ def self.new_ntoh(addr) | |||
|
|
|||
| # Convert a network byte ordered string form of an IP address into | |||
| # human readable form. | |||
| # It expects the string to be encoded in Encoding::US_ASCII (BINARY). | |||
There was a problem hiding this comment.
Encoding::US_ASCII is different from ASCII_8BIT. US_ASCII is a 7-bit encoding where codes above 127 cannot be contained.
There was a problem hiding this comment.
copy&pasted wrong string 😓
`IPAddr.ntop` takes the binary representation of an IP address, whose length should be 4 or 16 *bytes* (not characters/codepoints). The current implementation accepts strings in any encoding, but for some values in non-BINARY encoding, it fails proper length check and raises an `AddressFamilyError`. Since passing strings in a multibyte encoding has never worked correctly for years, this patch makes it an explicit error with an `InvalidAddressError`. Fixes: ruby#56
hanazuki
force-pushed
the
ntop-bytesize
branch
from
be01430
to
a33fd14
Compare
knu
approved these changes
Dec 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IPAddr.ntoptakes the binary representation of an IP address, whose length should be 4 or 16 bytes (not characters/codepoints).This patch fixes the bug that
IPAddr.ntopdoesn't handle Strings in a multibyte encoding correctly.Fixes: #56