3:38 p.m.
I noticed the virSocjetAddr() stuff when I was writing code that was
going to use virSocketAddrMask(). I think I ended up not using that
function after all, but the fixes are still worthwhile.
Laine Stump (4):
network: fix argument order/log level in message about
firewall_backend
util: fix virSocketAddrMask() when source and result are the same
object
util: make virSocketAddrIPv4 a union
util: use uint32 instead of char[4] for several virSocketAddrIPv4
operations
src/network/bridge_driver_conf.c | 4 +-
src/util/virsocketaddr.c | 84 +++++++++++++++-----------------
2 files changed, 42 insertions(+), 46 deletions(-)
--
2.46.0
3:38 p.m.
New subject: [PATCH 1/4] network: fix argument order/log level in message about firewall_backend
Oops.
Fixes: 64b966558cc6002fe150a0292a24eb2802a792c5
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
index 9da5e790b7..4e40286ee3 100644
--- a/src/network/bridge_driver_conf.c
+++ b/src/network/bridge_driver_conf.c
@@ -101,8 +101,8 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
fwBackendStr, filename);
return -1;
}
- VIR_INFO("firewall_backend setting requested from config file %s:
'%s'",
- virFirewallBackendTypeToString(fwBackends[0]), filename);
+ VIR_DEBUG("firewall_backend setting requested from config file %s:
'%s'",
+ filename, virFirewallBackendTypeToString(fwBackends[0]));
}
}
--
2.46.0
2:55 a.m.
New subject: [PATCH 1/4] network: fix argument order/log level in message
about firewall_backend
On a Thursday in 2024, Laine Stump wrote:
Oops.
Fixes: 64b966558cc6002fe150a0292a24eb2802a792c5
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
3:38 p.m.
New subject: [PATCH 2/4] util: fix virSocketAddrMask() when source and result are the same object
Many years ago (2011), virSocketAddrMask() had caused a bug by failing
to initialize an IPv6-specific field in the result virSocketAddr. This
was fixed by memset(0)ing the entire result (*network) at the
beginning of the function (thus making sure anything and everything
was initialized).
The problem is that virSocketAddrMask() has a comment above it that
says that the source (addr) and destination (network) arguments can
point to the same virSocketAddr. But in that case, the
memset(*network, 0) at the top of the function is actually doing a
memset(*addr, 0), and so there is nothing left for all the assignments
to copy except a giant field of 0's.
Fortunately in the 13 years since the memset was added, nobody has
ever called virSocketAddrMask() with addr and network being the same.
This patch makes the code agree with the comment by copying/masking
into a local virSocketAddr (which is initialized to all 0!) and then
copying that to *network after it's finished assigning things from
addr.
Fixes: ba08c5932e556aa4f5101357127a6224c40e5ebe
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virsocketaddr.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 0a1ae1ac5f..60d8071da7 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -685,31 +685,34 @@ virSocketAddrMask(const virSocketAddr *addr,
const virSocketAddr *netmask,
virSocketAddr *network)
{
- memset(network, 0, sizeof(*network));
+ virSocketAddr tmp = { 0 };
+
if (addr->data.stor.ss_family != netmask->data.stor.ss_family) {
network->data.stor.ss_family = AF_UNSPEC;
return -1;
}
if (addr->data.stor.ss_family == AF_INET) {
- network->data.inet4.sin_addr.s_addr
+ tmp.data.inet4.sin_addr.s_addr
= (addr->data.inet4.sin_addr.s_addr
& netmask->data.inet4.sin_addr.s_addr);
- network->data.inet4.sin_port = 0;
- network->data.stor.ss_family = AF_INET;
- network->len = addr->len;
+ tmp.data.inet4.sin_port = 0;
+ tmp.data.stor.ss_family = AF_INET;
+ tmp.len = addr->len;
+ *network = tmp;
return 0;
}
if (addr->data.stor.ss_family == AF_INET6) {
size_t i;
for (i = 0; i < 16; i++) {
- network->data.inet6.sin6_addr.s6_addr[i]
+ tmp.data.inet6.sin6_addr.s6_addr[i]
= (addr->data.inet6.sin6_addr.s6_addr[i]
& netmask->data.inet6.sin6_addr.s6_addr[i]);
}
- network->data.inet6.sin6_port = 0;
- network->data.stor.ss_family = AF_INET6;
- network->len = addr->len;
+ tmp.data.inet6.sin6_port = 0;
+ tmp.data.stor.ss_family = AF_INET6;
+ tmp.len = addr->len;
+ *network = tmp;
return 0;
}
network->data.stor.ss_family = AF_UNSPEC;
--
2.46.0
2:57 a.m.
New subject: [PATCH 2/4] util: fix virSocketAddrMask() when source and result
are the same object
On a Thursday in 2024, Laine Stump wrote:
Many years ago (2011), virSocketAddrMask() had caused a bug by
failing
to initialize an IPv6-specific field in the result virSocketAddr. This
was fixed by memset(0)ing the entire result (*network) at the
beginning of the function (thus making sure anything and everything
was initialized).
The problem is that virSocketAddrMask() has a comment above it that
says that the source (addr) and destination (network) arguments can
point to the same virSocketAddr. But in that case, the
memset(*network, 0) at the top of the function is actually doing a
memset(*addr, 0), and so there is nothing left for all the assignments
to copy except a giant field of 0's.
Fortunately in the 13 years since the memset was added, nobody has
ever called virSocketAddrMask() with addr and network being the same.
Would they ever need to? It might be simpler to just drop the comment.
This patch makes the code agree with the comment by copying/masking
into a local virSocketAddr (which is initialized to all 0!) and then
0! = 1
Either drop the exclamation mark, or spell out 0 ;)
copying that to *network after it's finished assigning things
from
addr.
Fixes: ba08c5932e556aa4f5101357127a6224c40e5ebe
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virsocketaddr.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
6:39 a.m.
New subject: [PATCH 2/4] util: fix virSocketAddrMask() when source and result
are the same object
On 9/20/24 3:57 AM, Ján Tomko wrote:
On a Thursday in 2024, Laine Stump wrote:
> Many years ago (2011), virSocketAddrMask() had caused a bug by failing
> to initialize an IPv6-specific field in the result virSocketAddr. This
> was fixed by memset(0)ing the entire result (*network) at the
> beginning of the function (thus making sure anything and everything
> was initialized).
>
> The problem is that virSocketAddrMask() has a comment above it that
> says that the source (addr) and destination (network) arguments can
> point to the same virSocketAddr. But in that case, the
> memset(*network, 0) at the top of the function is actually doing a
> memset(*addr, 0), and so there is nothing left for all the assignments
> to copy except a giant field of 0's.
>
> Fortunately in the 13 years since the memset was added, nobody has
> ever called virSocketAddrMask() with addr and network being the same.
>
Would they ever need to? It might be simpler to just drop the comment.
I actually made the fix because I had written code that used it in the
currently documented manner, made this patch when it didn't work, but
then decided to go a different way with my code. This gives (me) enough
of a hint that the documented way is at least "good to have".
> This patch makes the code agree with the comment by copying/masking
> into a local virSocketAddr (which is initialized to all 0!) and then
0! = 1
Either drop the exclamation mark, or spell out 0 ;)
So is that a winking smiley face with a halo floating above it, or a
winking surprised/shocked face with a unibrow that is also raised in
surprise? Maybe you should spell out which you mean so there's no
confusion :-P
> copying that to *network after it's finished assigning things from
> addr.
>
> Fixes: ba08c5932e556aa4f5101357127a6224c40e5ebe
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/util/virsocketaddr.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
3:38 p.m.
New subject: [PATCH 3/4] util: make virSocketAddrIPv4 a union
virSocketAddrIPv4 is a type used only internally by
virsocketaddr.c. It is defined to be a character array, which leads to
multiple occurences of extra bit fiddling and byte swapping for no
good reason (except to confuse).
An IPv4 address is really just a uint32_t with the bytes in network
order, which is exactly the type of the s_addr member of the
sockaddr_in that is a part of the publicly consumed struct
virSocketAddr, and that we are copying in and out of a
virSocketAddrIPv4. Sometimes it's simpler to just treat it as a
network-order uint32_t, so let's make our virSocketAddrIPv4 a union
that has both an unsigned char bytes[4] (for the times when we need to
look one byte at a time) and a uint32_t val (for the times when it's
simpler to treat it as a single value).
For now we just change all the uses from, e.g. x[i] to x.bytes[y];
an upcoming patch will simplify some of the code to remove loops by
using x.val instead of x.bytes when appropriate.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virsocketaddr.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 60d8071da7..4180fa1282 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -28,7 +28,11 @@
* Helpers to extract the IP arrays from the virSocketAddr *
* That part is the less portable of the module
*/
-typedef unsigned char virSocketAddrIPv4[4];
+typedef union {
+ uint32_t val;
+ unsigned char bytes[4];
+} virSocketAddrIPv4;
+
typedef unsigned short virSocketAddrIPv6[8];
typedef unsigned char virSocketAddrIPv6Bytes[16];
typedef unsigned char virSocketAddrIPv6Nibbles[32];
@@ -46,7 +50,7 @@ virSocketAddrGetIPv4Addr(const virSocketAddr *addr,
val = ntohl(addr->data.inet4.sin_addr.s_addr);
for (i = 0; i < 4; i++) {
- (*tab)[3 - i] = val & 0xFF;
+ tab->bytes[3 - i] = val & 0xFF;
val >>= 8;
}
@@ -838,7 +842,7 @@ int virSocketAddrCheckNetmask(virSocketAddr *addr1, virSocketAddr
*addr2,
return -1;
for (i = 0; i < 4; i++) {
- if ((t1[i] & tm[i]) != (t2[i] & tm[i]))
+ if ((t1.bytes[i] & tm.bytes[i]) != (t2.bytes[i] & tm.bytes[i]))
return 0;
}
@@ -986,14 +990,14 @@ virSocketAddrGetRange(virSocketAddr *start, virSocketAddr *end,
* are the same
*/
for (i = 0; i < 2; i++) {
- if (t1[i] != t2[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;
}
}
- ret = (t2[2] - t1[2]) * 256 + (t2[3] - t1[3]);
+ ret = (t2.bytes[2] - t1.bytes[2]) * 256 + (t2.bytes[3] - t1.bytes[3]);
if (ret < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("range %1$s - %2$s is reversed "),
@@ -1064,7 +1068,7 @@ int virSocketAddrGetNumNetmaskBits(const virSocketAddr *netmask)
return -1;
for (i = 0; i < 4; i++)
- if (tm[i] == 0xff)
+ if (tm.bytes[i] == 0xff)
c += 8;
else
break;
@@ -1075,7 +1079,7 @@ int virSocketAddrGetNumNetmaskBits(const virSocketAddr *netmask)
j = i << 3;
while (j < (8 * 4)) {
bit = 1 << (7 - (j & 7));
- if ((tm[j >> 3] & bit))
+ if ((tm.bytes[j >> 3] & bit))
c++;
else
break;
@@ -1084,7 +1088,7 @@ int virSocketAddrGetNumNetmaskBits(const virSocketAddr *netmask)
while (j < (8 * 4)) {
bit = 1 << (7 - (j & 7));
- if ((tm[j >> 3] & bit))
+ if ((tm.bytes[j >> 3] & bit))
return -1;
j++;
}
@@ -1326,7 +1330,7 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
return -1;
for (i = prefix / 8; i > 0; i--)
- virBufferAsprintf(&buf, "%u.", ip[i - 1]);
+ virBufferAsprintf(&buf, "%u.", ip.bytes[i - 1]);
virBufferAddLit(&buf, VIR_SOCKET_ADDR_IPV4_ARPA);
} else if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6)) {
@@ -1389,7 +1393,7 @@ virSocketAddrBytes(const virSocketAddr *addr,
return 0;
virSocketAddrGetIPv4Addr(addr, &ip);
- memcpy(bytes, ip, sizeof(ip));
+ memcpy(bytes, &ip, sizeof(ip));
return sizeof(ip);
}
--
2.46.0
2:58 a.m.
New subject: [PATCH 3/4] util: make virSocketAddrIPv4 a union
On a Thursday in 2024, Laine Stump wrote:
virSocketAddrIPv4 is a type used only internally by
virsocketaddr.c. It is defined to be a character array, which leads to
multiple occurences of extra bit fiddling and byte swapping for no
good reason (except to confuse).
An IPv4 address is really just a uint32_t with the bytes in network
order, which is exactly the type of the s_addr member of the
sockaddr_in that is a part of the publicly consumed struct
virSocketAddr, and that we are copying in and out of a
virSocketAddrIPv4. Sometimes it's simpler to just treat it as a
network-order uint32_t, so let's make our virSocketAddrIPv4 a union
that has both an unsigned char bytes[4] (for the times when we need to
look one byte at a time) and a uint32_t val (for the times when it's
simpler to treat it as a single value).
For now we just change all the uses from, e.g. x[i] to x.bytes[y];
an upcoming patch will simplify some of the code to remove loops by
using x.val instead of x.bytes when appropriate.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virsocketaddr.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
3:38 p.m.
New subject: [PATCH 4/4] util: use uint32 instead of char[4] for several virSocketAddrIPv4 operations
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(a)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.0
3:12 a.m.
New subject: [PATCH 4/4] util: use uint32 instead of char[4] for several
virSocketAddrIPv4 operations
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(a)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(a)redhat.com>
Jano
1:47 p.m.
New subject: [PATCH 4/4] util: use uint32 instead of char[4] for several
virSocketAddrIPv4 operations
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!
2:17 p.m.
New subject: [PATCH 4/4] util: use uint32 instead of char[4] for several
virSocketAddrIPv4 operations
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. :-(
92
days inactive
94
days old
11 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump