Re: [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