[PATCH 0/4] small improvements to virSocketAddr, and a trivial debug log change
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
Oops. Fixes: 64b966558cc6002fe150a0292a24eb2802a792c5 Signed-off-by: Laine Stump <laine@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
On a Thursday in 2024, Laine Stump wrote:
Oops.
Fixes: 64b966558cc6002fe150a0292a24eb2802a792c5 Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@redhat.com> --- src/util/virsocketaddr.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- src/util/virsocketaddr.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
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@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
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@redhat.com> --- src/util/virsocketaddr.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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.0
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@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. :-(
participants (2)
-
Ján Tomko -
Laine Stump