The previous check was all wrong since it calculated the how long would
the netlink message be if the netlink header was the payload and then
subtracted that from the whole message length, a variable that was not
used later in the code.  This check can fail if there are no additional
payloads, struct rtattr in particular, which we are parsing later,
however the RTA_OK macro would've caught that anyway.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virarptable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index d8e41c5a8668..8e805fb35332 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -84,7 +84,7 @@ virArpTableGet(void)
         int len = nh->nlmsg_len;
         void *addr;
 
-        if ((len -= NLMSG_LENGTH(sizeof(*nh))) < 0) {
+        if (len < NLMSG_SPACE(sizeof(*r))) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("wrong nlmsg len"));
             goto cleanup;
-- 
2.46.0

On 8/16/24 8:45 AM, Martin Kletzander wrote:
> The previous check was all wrong since it calculated the how long would
> the netlink message be if the netlink header was the payload and then
> subtracted that from the whole message length, a variable that was not
> used later in the code.  This check can fail if there are no additional
> payloads, struct rtattr in particular, which we are parsing later,
> however the RTA_OK macro would've caught that anyway.

I've always thought that netlink was unnecessarily complicated, and the 
fact that this code existed for this long and nobody noticed it is just 
a symptom of that :-/

> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Fixes: a176d67cdfaf5b8237a7e3a80d8be0e6bdf2d8fd

> ---
>   src/util/virarptable.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virarptable.c b/src/util/virarptable.c
> index d8e41c5a8668..8e805fb35332 100644
> --- a/src/util/virarptable.c
> +++ b/src/util/virarptable.c
> @@ -84,7 +84,7 @@ virArpTableGet(void)
>           int len = nh->nlmsg_len;
>           void *addr;
>   
> -        if ((len -= NLMSG_LENGTH(sizeof(*nh))) < 0) {
> +        if (len < NLMSG_SPACE(sizeof(*r))) {
>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                              _("wrong nlmsg len"));
>               goto cleanup;

I prefer Ondrej Mosnáček's suggested change here: 
https://bugzilla.redhat.com/2302245#c7 - he eliminates "len" entirely 
and replaces it with nh->nlmsg_len.

Reviewed-by: Laine Stump <laine@redhat.com>

(if you get rid of len)