| Message ID | 20241009091353.17419-1-demeng@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] qemu-ga: Fix some potential issues find by coverity | expand |
On Wed, Oct 09, 2024 at 05:13:53PM +0800, Dehan Meng wrote: > Key changes: > 1. Proper initialization of n to 0 for getline to function correctly. > 2. Avoiding freeing line prematurely. It's now only freed at the end of the function. > 3. sscanf return values are checked to ensure correct parsing. > 4. Variable declarations moved to the beginning of blocks. > 5. Followed the coding style of using snake_case for variable names. > 6. Merged redundant route and networkroute variables. Mixing multiple different bug fixes, style changes and arbitrary code refactoring into a single commit makes this unreviewable for correctness. Please split this up so there is a separate commit for each logically independent change > > Signed-off-by: Dehan Meng <demeng@redhat.com> > --- > qga/commands-linux.c | 139 ++++++++++++++++++++----------------------- > 1 file changed, 65 insertions(+), 74 deletions(-) > > diff --git a/qga/commands-linux.c b/qga/commands-linux.c > index 51d5e3d927..72a9548a06 100644 > --- a/qga/commands-linux.c > +++ b/qga/commands-linux.c > @@ -2094,26 +2094,28 @@ 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++) { > - 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); > > 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); > } > @@ -2122,103 +2124,92 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6) > GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp) > { > GuestNetworkRouteList *head = NULL, **tail = &head; > - const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"}; > + const char *route_files[] = {"/proc/net/route", "/proc/net/ipv6_route"}; > FILE *fp; > - size_t n; > + size_t n = 0; > char *line = NULL; > - int firstLine; > - int is_ipv6; > int i; > > for (i = 0; i < 2; i++) { > - firstLine = 1; > - is_ipv6 = (i == 1); > - fp = fopen(routeFiles[i], "r"); > + int first_line = 1; > + int is_ipv6 = (i == 1); > + fp = fopen(route_files[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; > } > > while (getline(&line, &n, fp) != -1) { > - if (firstLine && !is_ipv6) { > - firstLine = 0; > + if (first_line && !is_ipv6) { > + first_line = 0; > continue; > } > - GuestNetworkRoute *route = NULL; > - GuestNetworkRoute *networkroute; > - char Iface[IFNAMSIZ]; > + > + GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1); > + char iface[IFNAMSIZ]; > + > if (is_ipv6) { > - char Destination[33], Source[33], NextHop[33]; > - int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags; > + char destination[33], source[33], next_hop[33]; > + int des_prefixlen, src_prefixlen, metric, refcnt, use, flags; > > - /* Parse the line and extract the values */ > 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; > + 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; > + unsigned int destination, gateway, mask, flags; > + int refcnt, use, metric, mtu, window, irtt; > > - /* Parse the line and extract the values */ > 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); > } > > - free(line); > fclose(fp); > } > > + free(line); > return head; > } > -- > 2.40.1 > > With regards, Daniel
diff --git a/qga/commands-linux.c b/qga/commands-linux.c index 51d5e3d927..72a9548a06 100644 --- a/qga/commands-linux.c +++ b/qga/commands-linux.c @@ -2094,26 +2094,28 @@ 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++) { - 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); 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); } @@ -2122,103 +2124,92 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6) GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp) { GuestNetworkRouteList *head = NULL, **tail = &head; - const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"}; + const char *route_files[] = {"/proc/net/route", "/proc/net/ipv6_route"}; FILE *fp; - size_t n; + size_t n = 0; char *line = NULL; - int firstLine; - int is_ipv6; int i; for (i = 0; i < 2; i++) { - firstLine = 1; - is_ipv6 = (i == 1); - fp = fopen(routeFiles[i], "r"); + int first_line = 1; + int is_ipv6 = (i == 1); + fp = fopen(route_files[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; } while (getline(&line, &n, fp) != -1) { - if (firstLine && !is_ipv6) { - firstLine = 0; + if (first_line && !is_ipv6) { + first_line = 0; continue; } - GuestNetworkRoute *route = NULL; - GuestNetworkRoute *networkroute; - char Iface[IFNAMSIZ]; + + GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1); + char iface[IFNAMSIZ]; + if (is_ipv6) { - char Destination[33], Source[33], NextHop[33]; - int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags; + char destination[33], source[33], next_hop[33]; + int des_prefixlen, src_prefixlen, metric, refcnt, use, flags; - /* Parse the line and extract the values */ 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; + 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; + unsigned int destination, gateway, mask, flags; + int refcnt, use, metric, mtu, window, irtt; - /* Parse the line and extract the values */ 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); } - free(line); fclose(fp); } + free(line); return head; }
Key changes: 1. Proper initialization of n to 0 for getline to function correctly. 2. Avoiding freeing line prematurely. It's now only freed at the end of the function. 3. sscanf return values are checked to ensure correct parsing. 4. Variable declarations moved to the beginning of blocks. 5. Followed the coding style of using snake_case for variable names. 6. Merged redundant route and networkroute variables. Signed-off-by: Dehan Meng <demeng@redhat.com> --- qga/commands-linux.c | 139 ++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 74 deletions(-)