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