On a Thursday in 2024, Laine Stump wrote:
These 3 functions are easier to understand, and more efficient, when the IPv4 address is viewed as a uint32 rather than an array of bytes. virsocketAddrGetIPv4Addr() has bothered me for a long time - it was
virSocket
doing ntohl of the address into a temporary uint32, and then a loop
one-by-one swapping the order of all the bytes back to network
order. Of course this only works as described on little-endian
architectures - on big-endian architectures the first assignment won't
swap the bytes' ordering, but the loop assumes the bytes are now in
little-endian order and "swaps them back", so the result will be
incorrect. (Do we not support any big-endian targets that would have
exposed this bug long before now??)
virSocketAddrCheckNetmask() was checking each byte of the two
addresses individually, when it could instead just do the operation
once on the full 32 bit values.
virSocketGetRange() was checking for "range > 65535" by seeing if the
first 2 bytes of the start and end were different, and then doing
arithmetic combining the lower two bytes (along with necessary bit
shifting to account for network byte order) to determine the exact
size of the range. Instead we can just get the ntohl of start & end,
and do the math directly.
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/util/virsocketaddr.c | 47 +++++++++++++++-------------------------
1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 4180fa1282..61fe820d73 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -41,18 +41,10 @@ static int
@@ -976,35 +966,34 @@ virSocketAddrGetRange(virSocketAddr *start, virSocketAddr *end,
}
if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) {
- virSocketAddrIPv4 t1, t2;
+ virSocketAddrIPv4 startv4, endv4;
+ uint32_t startHost, endHost;
- if (virSocketAddrGetIPv4Addr(start, &t1) < 0 ||
- virSocketAddrGetIPv4Addr(end, &t2) < 0) {
+ if (virSocketAddrGetIPv4Addr(start, &startv4) < 0 ||
+ virSocketAddrGetIPv4Addr(end, &endv4) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to get IPv4 address for start or end of range %1$s - %2$s"),
startStr, endStr);
return -1;
}
- /* legacy check that everything except the last two bytes
- * are the same
- */
- for (i = 0; i < 2; i++) {
- if (t1.bytes[i] != t2.bytes[i]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("range %1$s - %2$s is too large (> 65535)"),
- startStr, endStr);
- return -1;
- }
+ startHost = ntohl(startv4.val);
+ endHost = ntohl(endv4.val);
+
+ if (endHost - startHost > 65535) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("range %1$s - %2$s is too large (> 65535)"),
+ startStr, endStr);
+ return -1;
}
- ret = (t2.bytes[2] - t1.bytes[2]) * 256 + (t2.bytes[3] - t1.bytes[3]);
- if (ret < 0) {
+
+ if (endHost < startHost) {
This check needs to happen before you subtract two unsigned integers, otherwise the substraction above overflows and this is essentially dead code. Before this patch: $ VIR_TEST_RANGE=48 VIR_TEST_DEBUG=1 ./run tests/sockettest TEST: sockettest 48) Test range 192.168.122.20 -> 192.168.122.1(192.168.122.1/24) size -1 ... libvirt: error : internal error: range 192.168.122.20 - 192.168.122.1 is reversed OK After: $ VIR_TEST_RANGE=48 VIR_TEST_DEBUG=1 ./run tests/sockettest TEST: sockettest 48) Test range 192.168.122.20 -> 192.168.122.1(192.168.122.1/24) size -1 ... libvirt: error : internal error: range 192.168.122.20 - 192.168.122.1 is too large (> 65535) OK
virReportError(VIR_ERR_INTERNAL_ERROR,
_("range %1$s - %2$s is reversed "),
startStr, endStr);
return -1;
}
- ret++;
+ ret = endHost - startHost + 1;
} else if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET6)) {
virSocketAddrIPv6 t1, t2;
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx> Jano
Attachment:
signature.asc
Description: PGP signature
