ipnetwork
ipnetwork copied to clipboard
IpNetwork::size() overflows if the prefix is 0
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?
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 justOption<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 deprecatesize
completely and have users compute this themselves. After all both the*_BITS
constant and the prefix are public.
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;
}
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.
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.
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.
I think #179 may fix this?