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
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@redhat.com>
---
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
virSocketAddrGetIPv4Addr(const virSocketAddr *addr,
virSocketAddrIPv4 *tab)
{
- unsigned long val;
- size_t i;
-
if (!addr || !tab || addr->data.stor.ss_family != AF_INET)
return -1;
- val = ntohl(addr->data.inet4.sin_addr.s_addr);
-
- for (i = 0; i < 4; i++) {
- tab->bytes[3 - i] = val & 0xFF;
- val >>= 8;
- }
+ tab->val = addr->data.inet4.sin_addr.s_addr;
return 0;
}
@@ -841,10 +833,8 @@ int virSocketAddrCheckNetmask(virSocketAddr *addr1, virSocketAddr *addr2,
(virSocketAddrGetIPv4Addr(netmask, &tm) < 0))
return -1;
- for (i = 0; i < 4; i++) {
- if ((t1.bytes[i] & tm.bytes[i]) != (t2.bytes[i] & tm.bytes[i]))
- return 0;
- }
+ if ((t1.val & tm.val) != (t2.val & tm.val))
+ return 0;
} else if (addr1->data.stor.ss_family == AF_INET6) {
virSocketAddrIPv6 t1, t2, tm;
@@ -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) {
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;
--
2.46.0On 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@redhat.com>
>---
> 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@redhat.com>
Jano
On 9/20/24 4:12 AM, Ján Tomko wrote:
> On a Thursday in 2024, Laine Stump wrote:
>> [...]
>> + 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
Ooh! Nice catch! I hadn't noticed because I was putting too much faith
in the unit tests, and they had "successfully failed as expected" both
before and after and I didn't look at the specific failure to notice
that it was failing in a different way. I fixed it before pushing.
Thanks for the reviews!
On 9/21/24 2:47 PM, Laine Stump wrote: > > Thanks for the reviews! Oops! Then I went and forgot to add the Reviewed-by: tag to the patches before I pushed them :-/. Now your stats will be off by 4. :-(
© 2016 - 2026 Red Hat, Inc.