* [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity
@ 2024-10-11 3:19 Dehan Meng
2024-10-11 3:19 ` [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing Dehan Meng
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Dehan Meng @ 2024-10-11 3:19 UTC (permalink / raw)
To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing. 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng @ 2024-10-11 3:19 ` Dehan Meng 2024-10-14 9:55 ` Daniel P. Berrangé 2024-10-11 3:19 ` [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly Dehan Meng ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Dehan Meng @ 2024-10-11 3:19 UTC (permalink / raw) To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing. 2024-10-11 3:19 ` [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing Dehan Meng @ 2024-10-14 9:55 ` Daniel P. Berrangé 0 siblings, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-10-14 9:55 UTC (permalink / raw) To: Dehan Meng; +Cc: qemu-devel, kkostiuk, michael.roth, peter.maydell 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly. 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng 2024-10-11 3:19 ` [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing Dehan Meng @ 2024-10-11 3:19 ` Dehan Meng 2024-10-14 9:57 ` Daniel P. Berrangé 2024-10-11 3:19 ` [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function Dehan Meng ` (2 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Dehan Meng @ 2024-10-11 3:19 UTC (permalink / raw) To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly. 2024-10-11 3:19 ` [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly Dehan Meng @ 2024-10-14 9:57 ` Daniel P. Berrangé 0 siblings, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-10-14 9:57 UTC (permalink / raw) To: Dehan Meng; +Cc: qemu-devel, kkostiuk, michael.roth, peter.maydell On Fri, Oct 11, 2024 at 11:19:35AM +0800, Dehan Meng wrote: > 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> > > 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 > > 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function. 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng 2024-10-11 3:19 ` [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing Dehan Meng 2024-10-11 3:19 ` [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly Dehan Meng @ 2024-10-11 3:19 ` Dehan Meng 2024-10-14 9:59 ` Daniel P. Berrangé 2024-10-11 3:19 ` [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 2024-10-11 16:12 ` [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Konstantin Kostiuk 4 siblings, 1 reply; 11+ messages in thread From: Dehan Meng @ 2024-10-11 3:19 UTC (permalink / raw) To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function. 2024-10-11 3:19 ` [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function Dehan Meng @ 2024-10-14 9:59 ` Daniel P. Berrangé 0 siblings, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-10-14 9:59 UTC (permalink / raw) To: Dehan Meng; +Cc: qemu-devel, kkostiuk, michael.roth, peter.maydell The subject line is too long - put the second sentance on a line of its own in the commit message, separated from the 1st line by a blank line. On Fri, Oct 11, 2024 at 11:19:36AM +0800, Dehan Meng wrote: > 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 > > 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* [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. 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng ` (2 preceding siblings ...) 2024-10-11 3:19 ` [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function Dehan Meng @ 2024-10-11 3:19 ` Dehan Meng 2024-10-14 10:06 ` Daniel P. Berrangé 2024-10-11 16:12 ` [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Konstantin Kostiuk 4 siblings, 1 reply; 11+ messages in thread From: Dehan Meng @ 2024-10-11 3:19 UTC (permalink / raw) To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [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. 2024-10-11 3:19 ` [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 @ 2024-10-14 10:06 ` Daniel P. Berrangé 0 siblings, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-10-14 10:06 UTC (permalink / raw) To: Dehan Meng; +Cc: qemu-devel, kkostiuk, michael.roth, peter.maydell Same comment about $SUBJECT being too long. You need a blank line after the 1st line in the commit message. On Fri, Oct 11, 2024 at 11:19:37AM +0800, Dehan Meng wrote: > 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); By moving this up to the time of declaration of 'route', you've introduced a memory leak when the above 'sscanf' line triggers the 'continue' branch. > - 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); Again, introduced a memory leak for the same reason. > - 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 > > 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 :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng ` (3 preceding siblings ...) 2024-10-11 3:19 ` [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 @ 2024-10-11 16:12 ` Konstantin Kostiuk 4 siblings, 0 replies; 11+ messages in thread From: Konstantin Kostiuk @ 2024-10-11 16:12 UTC (permalink / raw) To: Dehan Meng; +Cc: qemu-devel, michael.roth, peter.maydell [-- Attachment #1: Type: text/plain, Size: 1173 bytes --] On Fri, Oct 11, 2024 at 6:19 AM Dehan Meng <demeng@redhat.com> wrote: > 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. > > First of all, don't use so long commit title. You can add more info in the commit message if needed. Commits 3 and 4 have very long titles. Do not set . at the end of the commit title. As in git, we do not see cover-messages, please add `qemu-ga:` suffix for each commit Best Regards, Konstantin Kostiuk. > Signed-off-by: Dehan Meng <demeng@redhat.com> > > qga/commands-linux.c | 128 ++++++++++++++++++++----------------------- > 1 file changed, 59 insertions(+), 69 deletions(-) > > -- > 2.40.1 > > [-- Attachment #2: Type: text/html, Size: 1937 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity
@ 2024-10-21 13:28 Dehan Meng
0 siblings, 0 replies; 11+ messages in thread
From: Dehan Meng @ 2024-10-21 13:28 UTC (permalink / raw)
To: qemu-devel; +Cc: demeng, kkostiuk, michael.roth, peter.maydell, berrange
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
^ permalink raw reply [flat|nested] 11+ messages in threadend of thread, other threads:[~2024-10-21 13:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-11 3:19 [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Dehan Meng 2024-10-11 3:19 ` [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing Dehan Meng 2024-10-14 9:55 ` Daniel P. Berrangé 2024-10-11 3:19 ` [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly Dehan Meng 2024-10-14 9:57 ` Daniel P. Berrangé 2024-10-11 3:19 ` [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function Dehan Meng 2024-10-14 9:59 ` Daniel P. Berrangé 2024-10-11 3:19 ` [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 2024-10-14 10:06 ` Daniel P. Berrangé 2024-10-11 16:12 ` [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity Konstantin Kostiuk -- strict thread matches above, loose matches on Subject: below -- 2024-10-21 13:28 Dehan Meng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).