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

Ipaddr#native must also coerce @mask_addr #34

Merged
merged 1 commit into from Dec 3, 2021

Conversation

casperisfine
Copy link

Before it would be left as an IPv6 mask causing to_range to fail.

>> IPAddr.new("::2").native.to_range
/opt/rubies/3.0.3/lib/ruby/3.0.0/ipaddr.rb:479:in `set': invalid address (IPAddr::InvalidAddressError)

This bug seem quite old but it was hidden by #31

@hsbt @jeremyevans

@jeremyevans
Copy link
Contributor

If this only affects native, maybe the change should be made in native and not set? I'm not sure what the ramifications of modifying set are, I wouldn't want a change to set to hide other bugs. Here's a potential implementation:

diff --git a/lib/ipaddr.rb b/lib/ipaddr.rb
index 5df798f..664d23b 100644
--- a/lib/ipaddr.rb
+++ b/lib/ipaddr.rb
@@ -340,7 +340,9 @@ class IPAddr
     if !ipv4_mapped? && !_ipv4_compat?
       return self
     end
-    return self.clone.set(@addr & IN4MASK, Socket::AF_INET)
+    clone = self.clone
+    clone.instance_variable_set(:@mask_addr, @mask_addr & IN4MASK)
+    clone.set(@addr & IN4MASK, Socket::AF_INET)
   end

   # Returns a string for DNS reverse lookup.  It returns a string in

What do you think?

@casperisfine
Copy link
Author

What do you think?

Well, it seemed cleaner to do it from inside the cloned instance. The error scenario I know about involve native, but I don't know the library enough to know wether there are other cases where the @family may change.

To me it simply made sense encapsulation wise to do this casting in the method where @family is updated, as one is the consequence of the other.

But up to you, if you feel strongly about it I can change, but that seems like being overly cautious at the expense of code clarity to me.

@casperisfine
Copy link
Author

Eventually I could move it to right after: @family = family[0] so that it is only called when the family is changed.

@jeremyevans
Copy link
Contributor

Moving the change to @mask_addr to inside the if family[0] block sounds good. That way it only affects cases where a change to the family is being requested.

Before it would be left as an IPv6 mask causing `to_range` to fail.

```
>> IPAddr.new("::2").native.to_range
/opt/rubies/3.0.3/lib/ruby/3.0.0/ipaddr.rb:479:in `set': invalid address (IPAddr::InvalidAddressError)
```
@casperisfine
Copy link
Author

@jeremyevans done!

@byroot
Copy link
Member

byroot commented Dec 3, 2021

Thanks! I'll merge on green.

@byroot
Copy link
Member

byroot commented Dec 3, 2021

Well, it's green minus the Truffle thing which already fails on master.

@byroot byroot merged commit 0a4f319 into ruby:master Dec 3, 2021
@casperisfine casperisfine deleted the v6-parsing-failures branch December 3, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants