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

IpNetwork::size() overflows if the prefix is 0 #130

Open
dlon opened this issue Apr 29, 2020 · 6 comments
Open

IpNetwork::size() overflows if the prefix is 0 #130

dlon opened this issue Apr 29, 2020 · 6 comments

Comments

@dlon
Copy link

dlon commented Apr 29, 2020

Ipv4Network::size panics if self.prefix == 0 since the result is too large (2^32):

pub fn size(self) -> u32 {
    let host_bits = u32::from(IPV4_BITS - self.prefix);
    (2 as u32).pow(host_bits)
}

This could have been fixed by using u64 if not for the fact that the same problem occurs in Ipv6Network::size, where the type cannot be made any larger.

If a Result cannot be returned, should the potential panic be documented?

@faern
Copy link
Contributor

faern commented Apr 29, 2020

Using u64 for Ipv4Network would solve the issue there. But it would make the API less symmetrical in a way since we need some other solution for Ipv6Network. I think the way forward is to first come up with the best possible solution for Ipv6Network and after that determine what's the best path for Ipv4Network.

Possible solutions to Ipv6Network:

  • Return Result<u128, TooLargeNetworkError> or just Option<u128.
  • Let it panic like now, but document it well under a ## Panics documentation section
  • Completely change how size works (and rename to not cause confusion). Since it's always a power of two we can just return the power. That will always fit and users can compute the size and get to handle the possible overflow how they see fit for their particular needs. Or possibly deprecate size completely and have users compute this themselves. After all both the *_BITS constant and the prefix are public.

@faern
Copy link
Contributor

faern commented Apr 29, 2020

I realize the BITS constants are not exposed from the crate. I think we should do this, no matter how size is handled. They should probably be associated constants:

impl Ipv4Network {
    pub const BITS: u8 = 32;
}

impl Ipv6Network {
    pub const BITS: u8 = 128;
}

@achanda
Copy link
Owner

achanda commented May 1, 2020

Sounds like the path of least resistance is to document the panic properly so that there are no surprises. I also agree that we should export BITS constants. I will get those changes done unless anyone objects. In the future, I think returning Result<u128, TooLargeNetworkError> sounds like a good idea.

@faern
Copy link
Contributor

faern commented May 1, 2020

Please mind that the current version only panics in debug mode. In release mode where overflows are not caught it just returns size 0. That is potentially even worse, since the return value is wrong.

If we go the path of having it panic. Then the pow method needs to be changed to checked_pow(...).expect("Too large network")

If we go the path of least resistance I think changing Ipv4 to return a u64 is the way to go. It did this earlier and it's the easiest way to just make it work for all networks.

@xxcodianxx
Copy link

This issue is still not patched, but the solution to either use checked_pow for overflow or return a u64 seems like a good idea.

As for Ipv6, the Result<u128, TooLargeNetworkError> or Option route seems like the most sensible decision in terms of clarity and elegance.

This problem with IpNetwork::size() also affects the IpNetwork::nth() and IpNetwork::iter() API's, so if possible this should be fixed ASAP.

@ctrlcctrlv
Copy link
Contributor

I think #179 may fix this?

This was referenced Apr 29, 2024
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

No branches or pull requests

5 participants