[PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 51d5e3d927..2c2b5f4ff2 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
         int i;
 
         for (i = 0; i < 16; i++) {
-            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
+            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
+                return NULL;
+            }
         }
         inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
 
-- 
2.40.1

Re: [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[Daniel P. Berrangé]

On Fri, Oct 11, 2024 at 11:19:34AM +0800, Dehan Meng wrote:
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 51d5e3d927..2c2b5f4ff2 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
>          int i;
>  
>          for (i = 0; i < 16; i++) {
> -            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
> +            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
> +                return NULL;
> +            }
>          }
>          inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);

None of the callers of this function are expecting it to return NULL.

eg this code:

                networkroute->destination = hexToIPAddress(Destination, 1);
                networkroute->metric = Metric;
                networkroute->source = hexToIPAddress(Source, 1);
                networkroute->desprefixlen = g_strdup_printf(
                    "%d", DesPrefixlen
                );
                networkroute->srcprefixlen = g_strdup_printf(
                    "%d", SrcPrefixlen
                );
                networkroute->nexthop = hexToIPAddress(NextHop, 1);

The QAPI schema allows 'source' and 'nexthop' to be optional so those
two are fnie.

The 'destination' field is marked as mandatory thoug, so must not
be NULL.

IOW, in the calls we need to check for NULL, and skip adding the
entire route object if 'destniation' is NULL.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity

[Dehan Meng]

v2:
Split v1 up to separate commits for each logically independent change

Dehan Meng (4):
  sscanf return values are checked to ensure correct parsing.
  Proper initialization of n to 0 for getline to function correctly.
  Avoiding freeing line prematurely. It's now only freed at the end of
    the function.
  For correcting code style: Variable declarations moved to the
    beginning of blocks Followed the coding style of using snake_case
    for variable names. And merged redundant route and networkroute
    variables.

Signed-off-by: Dehan Meng <demeng@redhat.com>

 qga/commands-linux.c | 128 ++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 69 deletions(-)

-- 
2.40.1

[PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 51d5e3d927..2c2b5f4ff2 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
         int i;
 
         for (i = 0; i < 16; i++) {
-            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
+            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
+                return NULL;
+            }
         }
         inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
 
-- 
2.40.1

[PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 2c2b5f4ff2..b905f33a57 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2126,7 +2126,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     GuestNetworkRouteList *head = NULL, **tail = &head;
     const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"};
     FILE *fp;
-    size_t n;
+    size_t n = 0;
     char *line = NULL;
     int firstLine;
     int is_ipv6;
-- 
2.40.1

[PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index b905f33a57..4f0e38be81 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2137,8 +2137,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
         is_ipv6 = (i == 1);
         fp = fopen(routeFiles[i], "r");
         if (fp == NULL) {
-            error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]);
-            free(line);
+            error_setg_errno(errp, errno, "open(\"%s\")", route_files[i]);
             continue;
         }
 
@@ -2218,9 +2217,8 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
             QAPI_LIST_APPEND(tail, route);
         }
 
-        free(line);
         fclose(fp);
     }
-
+    free(line);
     return head;
 }
-- 
2.40.1

[PATCH v2 4/4] For correcting code style: Variable declarations moved to the beginning of blocks Followed the coding style of using snake_case for variable names. And merged redundant route and networkroute variables.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 116 ++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 4f0e38be81..c6cca630ef 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2094,12 +2094,12 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
     return head;
 }
 
-static char *hexToIPAddress(const void *hexValue, int is_ipv6)
+static char *hex_to_ip_address(const void *hex_value, int is_ipv6)
 {
     if (is_ipv6) {
         char addr[INET6_ADDRSTRLEN];
         struct in6_addr in6;
-        const char *hexStr = (const char *)hexValue;
+        const char *hex_str = (const char *)hex_value;
         int i;
 
         for (i = 0; i < 16; i++) {
@@ -2111,11 +2111,11 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
 
         return g_strdup(addr);
     } else {
-        unsigned int hexInt = *(unsigned int *)hexValue;
-        unsigned int byte1 = (hexInt >> 24) & 0xFF;
-        unsigned int byte2 = (hexInt >> 16) & 0xFF;
-        unsigned int byte3 = (hexInt >> 8) & 0xFF;
-        unsigned int byte4 = hexInt & 0xFF;
+        unsigned int hex_int = *(unsigned int *)hex_value;
+        unsigned int byte1 = (hex_int >> 24) & 0xFF;
+        unsigned int byte2 = (hex_int >> 16) & 0xFF;
+        unsigned int byte3 = (hex_int >> 8) & 0xFF;
+        unsigned int byte4 = hex_int & 0xFF;
 
         return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1);
     }
@@ -2131,6 +2131,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     int firstLine;
     int is_ipv6;
     int i;
+    char iface[IFNAMSIZ];
 
     for (i = 0; i < 2; i++) {
         firstLine = 1;
@@ -2146,72 +2147,61 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 firstLine = 0;
                 continue;
             }
-            GuestNetworkRoute *route = NULL;
-            GuestNetworkRoute *networkroute;
-            char Iface[IFNAMSIZ];
-            if (is_ipv6) {
-                char Destination[33], Source[33], NextHop[33];
-                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
 
-                /* Parse the line and extract the values */
+            GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
+
+            if (is_ipv6) {
+                char destination[33], source[33], next_hop[33];
+                int des_prefixlen, src_prefixlen, metric, refcnt, use, flags;
                 if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
-                           Destination, &DesPrefixlen, Source,
-                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
-                           &Use, &Flags, Iface) != 10) {
+                           destination, &des_prefixlen, source,
+                           &src_prefixlen, next_hop, &metric, &refcnt,
+                           &use, &flags, iface) != 10) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(Destination, 1);
-                networkroute->metric = Metric;
-                networkroute->source = hexToIPAddress(Source, 1);
-                networkroute->desprefixlen = g_strdup_printf(
-                    "%d", DesPrefixlen
-                );
-                networkroute->srcprefixlen = g_strdup_printf(
-                    "%d", SrcPrefixlen
-                );
-                networkroute->nexthop = hexToIPAddress(NextHop, 1);
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->version = 6;
-            } else {
-                unsigned int Destination, Gateway, Mask, Flags;
-                int RefCnt, Use, Metric, MTU, Window, IRTT;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(destination, 1);
+                route->source = hex_to_ip_address(source, 1);
+                route->nexthop = hex_to_ip_address(next_hop, 1);
+                route->desprefixlen = g_strdup_printf("%d", des_prefixlen);
+                route->srcprefixlen = g_strdup_printf("%d", src_prefixlen);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->version = 6;
 
-                /* Parse the line and extract the values */
+            } else {
+                unsigned int destination, gateway, mask, flags;
+                int refcnt, use, metric, mtu, window, irtt;
                 if (sscanf(line, "%s %X %X %x %d %d %d %X %d %d %d",
-                           Iface, &Destination, &Gateway, &Flags, &RefCnt,
-                           &Use, &Metric, &Mask, &MTU, &Window, &IRTT) != 11) {
+                           iface, &destination, &gateway, &flags, &refcnt,
+                           &use, &metric, &mask, &mtu, &window, &irtt) != 11) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(&Destination, 0);
-                networkroute->gateway = hexToIPAddress(&Gateway, 0);
-                networkroute->mask = hexToIPAddress(&Mask, 0);
-                networkroute->metric = Metric;
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->has_mtu = true;
-                networkroute->mtu = MTU;
-                networkroute->has_window = true;
-                networkroute->window = Window;
-                networkroute->has_irtt = true;
-                networkroute->irtt = IRTT;
-                networkroute->version = 4;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(&destination, 0);
+                route->gateway = hex_to_ip_address(&gateway, 0);
+                route->mask = hex_to_ip_address(&mask, 0);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->has_mtu = true;
+                route->mtu = mtu;
+                route->has_window = true;
+                route->window = window;
+                route->has_irtt = true;
+                route->irtt = irtt;
+                route->version = 4;
             }
 
             QAPI_LIST_APPEND(tail, route);
-- 
2.40.1

[PATCH v3 0/4] qemu-ga: Modify commits message and make some minor code update

[Dehan Meng]

v3:
Modify commits message and do some minor update.

v2:
Split v1 up to separate commits for each logically independent change

Dehan Meng (4):
  'Null' check for mandatory parameters
  Initialize correctly so getline works properly
  Avoiding freeing line prematurely
  For correcting code style

Signed-off-by: Dehan Meng <demeng@redhat.com>

 qga/commands-linux.c | 139 ++++++++++++++++++++++---------------------
 1 file changed, 70 insertions(+), 69 deletions(-)

-- 
2.40.1

[PATCH v3 1/4] qemu-ga: 'Null' check for mandatory parameters

[Dehan Meng]

sscanf return values are checked and add 'Null' check for
mandatory parameters.

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 51d5e3d927..f0e9cdd27c 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
         int i;
 
         for (i = 0; i < 16; i++) {
-            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
+            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
+                return NULL;
+            }
         }
         inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
 
@@ -2164,6 +2166,10 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 networkroute = route;
                 networkroute->iface = g_strdup(Iface);
                 networkroute->destination = hexToIPAddress(Destination, 1);
+                if (networkroute->destination == NULL) {
+                    g_free(route);
+                    continue;
+                }
                 networkroute->metric = Metric;
                 networkroute->source = hexToIPAddress(Source, 1);
                 networkroute->desprefixlen = g_strdup_printf(
@@ -2195,6 +2201,10 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 networkroute = route;
                 networkroute->iface = g_strdup(Iface);
                 networkroute->destination = hexToIPAddress(&Destination, 0);
+                if (networkroute->destination == NULL) {
+                    g_free(route);
+                    continue;
+                }
                 networkroute->gateway = hexToIPAddress(&Gateway, 0);
                 networkroute->mask = hexToIPAddress(&Mask, 0);
                 networkroute->metric = Metric;
-- 
2.40.1

[PATCH v3 2/4] qemu-ga: Initialize correctly so getline works properly

[Dehan Meng]

Proper initialization of param 'size_t n' to '0' for
getline to function correctly.

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index f0e9cdd27c..32bf1b8ce7 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2126,7 +2126,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     GuestNetworkRouteList *head = NULL, **tail = &head;
     const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"};
     FILE *fp;
-    size_t n;
+    size_t n = 0;
     char *line = NULL;
     int firstLine;
     int is_ipv6;
-- 
2.40.1

[PATCH v3 3/4] qemu-ga: Avoiding freeing line prematurely

[Dehan Meng]

It's now only freed at the end of the function.

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 32bf1b8ce7..c6bda78de6 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2137,8 +2137,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
         is_ipv6 = (i == 1);
         fp = fopen(routeFiles[i], "r");
         if (fp == NULL) {
-            error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]);
-            free(line);
+            error_setg_errno(errp, errno, "open(\"%s\")", route_files[i]);
             continue;
         }
 
@@ -2226,9 +2225,8 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
             QAPI_LIST_APPEND(tail, route);
         }
 
-        free(line);
         fclose(fp);
     }
-
+    free(line);
     return head;
 }
-- 
2.40.1

[PATCH v3 4/4] qemu-ga: For correcting code style

[Dehan Meng]

Variable declarations moved to the beginning of blocks
Followed the coding style of using snake_case for variable names.
And merged redundant route and networkroute variables.

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 123 ++++++++++++++++++++-----------------------
 1 file changed, 58 insertions(+), 65 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index c6bda78de6..9fb31956b4 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2094,12 +2094,12 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
     return head;
 }
 
-static char *hexToIPAddress(const void *hexValue, int is_ipv6)
+static char *hex_to_ip_address(const void *hex_value, int is_ipv6)
 {
     if (is_ipv6) {
         char addr[INET6_ADDRSTRLEN];
         struct in6_addr in6;
-        const char *hexStr = (const char *)hexValue;
+        const char *hex_str = (const char *)hex_value;
         int i;
 
         for (i = 0; i < 16; i++) {
@@ -2111,11 +2111,11 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
 
         return g_strdup(addr);
     } else {
-        unsigned int hexInt = *(unsigned int *)hexValue;
-        unsigned int byte1 = (hexInt >> 24) & 0xFF;
-        unsigned int byte2 = (hexInt >> 16) & 0xFF;
-        unsigned int byte3 = (hexInt >> 8) & 0xFF;
-        unsigned int byte4 = hexInt & 0xFF;
+        unsigned int hex_int = *(unsigned int *)hex_value;
+        unsigned int byte1 = (hex_int >> 24) & 0xFF;
+        unsigned int byte2 = (hex_int >> 16) & 0xFF;
+        unsigned int byte3 = (hex_int >> 8) & 0xFF;
+        unsigned int byte4 = hex_int & 0xFF;
 
         return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1);
     }
@@ -2131,6 +2131,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     int firstLine;
     int is_ipv6;
     int i;
+    char iface[IFNAMSIZ];
 
     for (i = 0; i < 2; i++) {
         firstLine = 1;
@@ -2146,80 +2147,72 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 firstLine = 0;
                 continue;
             }
-            GuestNetworkRoute *route = NULL;
-            GuestNetworkRoute *networkroute;
-            char Iface[IFNAMSIZ];
-            if (is_ipv6) {
-                char Destination[33], Source[33], NextHop[33];
-                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
 
-                /* Parse the line and extract the values */
+            if (is_ipv6) {
+                char destination[33], source[33], next_hop[33];
+                int des_prefixlen, src_prefixlen, metric, refcnt, use, flags;
                 if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
-                           Destination, &DesPrefixlen, Source,
-                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
-                           &Use, &Flags, Iface) != 10) {
+                           destination, &des_prefixlen, source,
+                           &src_prefixlen, next_hop, &metric, &refcnt,
+                           &use, &flags, iface) != 10) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(Destination, 1);
-                if (networkroute->destination == NULL) {
+                GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
+
+                route->destination = hex_to_ip_address(destination, 1);
+                if (route->destination == NULL) {
                     g_free(route);
                     continue;
                 }
-                networkroute->metric = Metric;
-                networkroute->source = hexToIPAddress(Source, 1);
-                networkroute->desprefixlen = g_strdup_printf(
-                    "%d", DesPrefixlen
-                );
-                networkroute->srcprefixlen = g_strdup_printf(
-                    "%d", SrcPrefixlen
-                );
-                networkroute->nexthop = hexToIPAddress(NextHop, 1);
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->version = 6;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(destination, 1);
+                route->source = hex_to_ip_address(source, 1);
+                route->nexthop = hex_to_ip_address(next_hop, 1);
+                route->desprefixlen = g_strdup_printf("%d", des_prefixlen);
+                route->srcprefixlen = g_strdup_printf("%d", src_prefixlen);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->version = 6;
             } else {
-                unsigned int Destination, Gateway, Mask, Flags;
-                int RefCnt, Use, Metric, MTU, Window, IRTT;
-
-                /* Parse the line and extract the values */
+                unsigned int destination, gateway, mask, flags;
+                int refcnt, use, metric, mtu, window, irtt;
                 if (sscanf(line, "%s %X %X %x %d %d %d %X %d %d %d",
-                           Iface, &Destination, &Gateway, &Flags, &RefCnt,
-                           &Use, &Metric, &Mask, &MTU, &Window, &IRTT) != 11) {
+                           iface, &destination, &gateway, &flags, &refcnt,
+                           &use, &metric, &mask, &mtu, &window, &irtt) != 11) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(&Destination, 0);
-                if (networkroute->destination == NULL) {
+                GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
+
+                route->destination = hex_to_ip_address(destination, 1);
+                if (route->destination == NULL) {
                     g_free(route);
                     continue;
                 }
-                networkroute->gateway = hexToIPAddress(&Gateway, 0);
-                networkroute->mask = hexToIPAddress(&Mask, 0);
-                networkroute->metric = Metric;
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->has_mtu = true;
-                networkroute->mtu = MTU;
-                networkroute->has_window = true;
-                networkroute->window = Window;
-                networkroute->has_irtt = true;
-                networkroute->irtt = IRTT;
-                networkroute->version = 4;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(&destination, 0);
+                route->gateway = hex_to_ip_address(&gateway, 0);
+                route->mask = hex_to_ip_address(&mask, 0);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->has_mtu = true;
+                route->mtu = mtu;
+                route->has_window = true;
+                route->window = window;
+                route->has_irtt = true;
+                route->irtt = irtt;
+                route->version = 4;
             }
 
             QAPI_LIST_APPEND(tail, route);
-- 
2.40.1

Re: [PATCH v3 1/4] qemu-ga: 'Null' check for mandatory parameters

[Daniel P. Berrangé]

On Mon, Oct 21, 2024 at 09:28:36PM +0800, Dehan Meng wrote:
> sscanf return values are checked and add 'Null' check for
> mandatory parameters.
> 
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 51d5e3d927..f0e9cdd27c 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
>          int i;
>  
>          for (i = 0; i < 16; i++) {
> -            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
> +            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
> +                return NULL;
> +            }
>          }
>          inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
>  
> @@ -2164,6 +2166,10 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>                  networkroute = route;
>                  networkroute->iface = g_strdup(Iface);
>                  networkroute->destination = hexToIPAddress(Destination, 1);
> +                if (networkroute->destination == NULL) {
> +                    g_free(route);
> +                    continue;
> +                }

This is still leaking the 'networkroute->iface' string.

The existing code is a bit strange having 'route' and 'networkroute'
variables.

I'd suggest removing the "route" variable entirely.

Then have a code pattern that relies on g_autoptr to automatically
free the struct & all its fields.

eg something that looks approx like this:

   g_autoptr(GuestNetorkRoute) networkroute = NULL;

   ...
   
   if (is_ipv6) {
       ...
       networkroute = g_new0(GuestNetorkRoute, 1);
       networkroute->iface = g_strdup(Iface);
       networkroute->destination = hexToIPAddress(Destination, 1);
       if (networkroute->destination == NULL) {
          continue;
       }
       ...
   } else {
       ...
       networkroute = g_new0(GuestNetorkRoute, 1);
       networkroute->iface = g_strdup(Iface);
       networkroute->destination = hexToIPAddress(Destination, 1);
       if (networkroute->destination == NULL) {
          continue;
       }
       ...
   }

   QAPI_LIST_APPEND(tail, g_steal_pointer(&networkroute));
  

>                  networkroute->metric = Metric;
>                  networkroute->source = hexToIPAddress(Source, 1);
>                  networkroute->desprefixlen = g_strdup_printf(
> @@ -2195,6 +2201,10 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>                  networkroute = route;
>                  networkroute->iface = g_strdup(Iface);
>                  networkroute->destination = hexToIPAddress(&Destination, 0);
> +                if (networkroute->destination == NULL) {
> +                    g_free(route);
> +                    continue;
> +                }
>                  networkroute->gateway = hexToIPAddress(&Gateway, 0);
>                  networkroute->mask = hexToIPAddress(&Mask, 0);
>                  networkroute->metric = Metric;
> -- 
> 2.40.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/4] qemu-ga: Initialize correctly so getline works properly

[Daniel P. Berrangé]

On Mon, Oct 21, 2024 at 09:28:37PM +0800, Dehan Meng wrote:
> Proper initialization of param 'size_t n' to '0' for
> getline to function correctly.
> 
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 3/4] qemu-ga: Avoiding freeing line prematurely

[Daniel P. Berrangé]

On Mon, Oct 21, 2024 at 09:28:38PM +0800, Dehan Meng wrote:
> It's now only freed at the end of the function.
> 
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/4] qemu-ga: 'Null' check for mandatory parameters

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]

On Mon, Oct 21, 2024 at 7:45 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Oct 21, 2024 at 09:28:36PM +0800, Dehan Meng wrote:
> > sscanf return values are checked and add 'Null' check for
> > mandatory parameters.
> >
> > Signed-off-by: Dehan Meng <demeng@redhat.com>
> > ---
> >  qga/commands-linux.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> > index 51d5e3d927..f0e9cdd27c 100644
> > --- a/qga/commands-linux.c
> > +++ b/qga/commands-linux.c
> > @@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue,
> int is_ipv6)
> >          int i;
> >
> >          for (i = 0; i < 16; i++) {
> > -            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
> > +            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) !=
> 1) {
> > +                return NULL;
> > +            }
> >          }
> >          inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
> >
> > @@ -2164,6 +2166,10 @@ GuestNetworkRouteList
> *qmp_guest_network_get_route(Error **errp)
> >                  networkroute = route;
> >                  networkroute->iface = g_strdup(Iface);
> >                  networkroute->destination = hexToIPAddress(Destination,
> 1);
> > +                if (networkroute->destination == NULL) {
> > +                    g_free(route);
> > +                    continue;
> > +                }
>
> This is still leaking the 'networkroute->iface' string.
>
> The existing code is a bit strange having 'route' and 'networkroute'
> variables.
>
> I'd suggest removing the "route" variable entirely.
>

This part was done in patch 4/4


>
> Then have a code pattern that relies on g_autoptr to automatically
> free the struct & all its fields.
>

Agree with this


>
> eg something that looks approx like this:
>
>    g_autoptr(GuestNetorkRoute) networkroute = NULL;
>
>    ...
>
>    if (is_ipv6) {
>        ...
>        networkroute = g_new0(GuestNetorkRoute, 1);
>        networkroute->iface = g_strdup(Iface);
>        networkroute->destination = hexToIPAddress(Destination, 1);
>        if (networkroute->destination == NULL) {
>           continue;
>        }
>        ...
>    } else {
>        ...
>        networkroute = g_new0(GuestNetorkRoute, 1);
>        networkroute->iface = g_strdup(Iface);
>        networkroute->destination = hexToIPAddress(Destination, 1);
>        if (networkroute->destination == NULL) {
>           continue;
>        }
>        ...
>    }
>
>    QAPI_LIST_APPEND(tail, g_steal_pointer(&networkroute));
>
>
> >                  networkroute->metric = Metric;
> >                  networkroute->source = hexToIPAddress(Source, 1);
> >                  networkroute->desprefixlen = g_strdup_printf(
> > @@ -2195,6 +2201,10 @@ GuestNetworkRouteList
> *qmp_guest_network_get_route(Error **errp)
> >                  networkroute = route;
> >                  networkroute->iface = g_strdup(Iface);
> >                  networkroute->destination =
> hexToIPAddress(&Destination, 0);
> > +                if (networkroute->destination == NULL) {
> > +                    g_free(route);
> > +                    continue;
> > +                }
> >                  networkroute->gateway = hexToIPAddress(&Gateway, 0);
> >                  networkroute->mask = hexToIPAddress(&Mask, 0);
> >                  networkroute->metric = Metric;
> > --
> > 2.40.1
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 5856 bytes --]

Re: [PATCH v3 1/4] qemu-ga: 'Null' check for mandatory parameters

[Dehan Meng]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

okay, thanks all for the suggestions, I'll make changes to a new commit, so
that you can review the commit5 later.

On Tue, Oct 22, 2024 at 1:14 AM Konstantin Kostiuk <kkostiuk@redhat.com>
wrote:

>
>
> On Mon, Oct 21, 2024 at 7:45 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Mon, Oct 21, 2024 at 09:28:36PM +0800, Dehan Meng wrote:
>> > sscanf return values are checked and add 'Null' check for
>> > mandatory parameters.
>> >
>> > Signed-off-by: Dehan Meng <demeng@redhat.com>
>> > ---
>> >  qga/commands-linux.c | 12 +++++++++++-
>> >  1 file changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/qga/commands-linux.c b/qga/commands-linux.c
>> > index 51d5e3d927..f0e9cdd27c 100644
>> > --- a/qga/commands-linux.c
>> > +++ b/qga/commands-linux.c
>> > @@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue,
>> int is_ipv6)
>> >          int i;
>> >
>> >          for (i = 0; i < 16; i++) {
>> > -            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
>> > +            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) !=
>> 1) {
>> > +                return NULL;
>> > +            }
>> >          }
>> >          inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
>> >
>> > @@ -2164,6 +2166,10 @@ GuestNetworkRouteList
>> *qmp_guest_network_get_route(Error **errp)
>> >                  networkroute = route;
>> >                  networkroute->iface = g_strdup(Iface);
>> >                  networkroute->destination =
>> hexToIPAddress(Destination, 1);
>> > +                if (networkroute->destination == NULL) {
>> > +                    g_free(route);
>> > +                    continue;
>> > +                }
>>
>> This is still leaking the 'networkroute->iface' string.
>>
>> The existing code is a bit strange having 'route' and 'networkroute'
>> variables.
>>
>> I'd suggest removing the "route" variable entirely.
>>
>
> This part was done in patch 4/4
>
>
>>
>> Then have a code pattern that relies on g_autoptr to automatically
>> free the struct & all its fields.
>>
>
> Agree with this
>
>
>>
>> eg something that looks approx like this:
>>
>>    g_autoptr(GuestNetorkRoute) networkroute = NULL;
>>
>>    ...
>>
>>    if (is_ipv6) {
>>        ...
>>        networkroute = g_new0(GuestNetorkRoute, 1);
>>        networkroute->iface = g_strdup(Iface);
>>        networkroute->destination = hexToIPAddress(Destination, 1);
>>        if (networkroute->destination == NULL) {
>>           continue;
>>        }
>>        ...
>>    } else {
>>        ...
>>        networkroute = g_new0(GuestNetorkRoute, 1);
>>        networkroute->iface = g_strdup(Iface);
>>        networkroute->destination = hexToIPAddress(Destination, 1);
>>        if (networkroute->destination == NULL) {
>>           continue;
>>        }
>>        ...
>>    }
>>
>>    QAPI_LIST_APPEND(tail, g_steal_pointer(&networkroute));
>>
>>
>> >                  networkroute->metric = Metric;
>> >                  networkroute->source = hexToIPAddress(Source, 1);
>> >                  networkroute->desprefixlen = g_strdup_printf(
>> > @@ -2195,6 +2201,10 @@ GuestNetworkRouteList
>> *qmp_guest_network_get_route(Error **errp)
>> >                  networkroute = route;
>> >                  networkroute->iface = g_strdup(Iface);
>> >                  networkroute->destination =
>> hexToIPAddress(&Destination, 0);
>> > +                if (networkroute->destination == NULL) {
>> > +                    g_free(route);
>> > +                    continue;
>> > +                }
>> >                  networkroute->gateway = hexToIPAddress(&Gateway, 0);
>> >                  networkroute->mask = hexToIPAddress(&Mask, 0);
>> >                  networkroute->metric = Metric;
>> > --
>> > 2.40.1
>> >
>>
>> With regards,
>> Daniel
>> --
>> |: https://berrange.com      -o-
>> https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-
>> https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-
>> https://www.instagram.com/dberrange :|
>>
>>

[-- Attachment #2: Type: text/html, Size: 6341 bytes --]