achanda / ipnetwork

A library to work with CIDRs in rust
Apache License 2.0
121 stars 38 forks source link

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

Open dlon opened 4 years ago

dlon commented 4 years ago

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 commented 4 years ago

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:

faern commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

xcodian commented 2 years ago

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 commented 1 year ago

I think #179 may fix this?