| Message ID | 20240723070251.25575-26-kkostiuk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/25] qga: drop blocking of guest-get-memory-block-size command | expand |
On Tue, 23 Jul 2024 at 08:03, Konstantin Kostiuk <kkostiuk@redhat.com> wrote: > > From: Dehan Meng <demeng@redhat.com> > > The Route information of the Linux VM needs to be used > by administrators and users when debugging network problems > and troubleshooting. > > Signed-off-by: Dehan Meng <demeng@redhat.com> > Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com> > Message-ID: <20240613092802.346246-2-demeng@redhat.com> > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com> Hi; Coverity points out some potential issues with this commit: > +static char *hexToIPAddress(const void *hexValue, int is_ipv6) > +{ > + if (is_ipv6) { > + char addr[INET6_ADDRSTRLEN]; > + struct in6_addr in6; > + const char *hexStr = (const char *)hexValue; > + int i; > + > + for (i = 0; i < 16; i++) { > + sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]); We don't check the sscanf() return value here. (CID 1558558) > + } > + 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; > + > + return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1); > + } > +} > + > +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; > + char *line = NULL; > + int firstLine; > + int is_ipv6; > + int i; The handling of the getline() buffer in this function doesn't seem to be correct (CID 1558559). Firstly, the manpage says that to get the initial "allocate me a buffer", line must be NULL and also n must be 0, but we don't initialize n here. > + for (i = 0; i < 2; i++) { > + firstLine = 1; > + is_ipv6 = (i == 1); > + fp = fopen(routeFiles[i], "r"); > + if (fp == NULL) { > + error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]); > + free(line); Here we free() line, but we continue the for() loop. So next time around the loop (assuming the second fopen succeeds) we'll pass line to getline() and it will be a non-NULL pointer to freed memory. Is this error case supposed to exit the for() loop entirely instead of continuing? Either way, it shouldn't free(line) here I think. > + continue; > + } > + > + while (getline(&line, &n, fp) != -1) { > + if (firstLine && !is_ipv6) { > + firstLine = 0; > + continue; > + } > + GuestNetworkRoute *route = NULL; > + GuestNetworkRoute *networkroute; > + char Iface[IFNAMSIZ]; Our coding style says you shouldn't declare variables in the middle of a block. Coding style also says variable names are lowercase with underscores, not CamelCase. (CamelCase is for typenames.) > + 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 (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s", > + Destination, &DesPrefixlen, Source, > + &SrcPrefixlen, NextHop, &Metric, &RefCnt, > + &Use, &Flags, Iface) != 10) { > + continue; > + } > + > + route = g_new0(GuestNetworkRoute, 1); > + networkroute = route; Why do we have separate "route" and "networkroute" variables here? As far as I can see they are identical and can be merged. > + 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; > + > + /* 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) { > + 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; > + } > + > + QAPI_LIST_APPEND(tail, route); > + } > + > + free(line); Similarly here we free(line) but next time around the for() loop we'll pass it to getline anyway. > + fclose(fp); > + } Since getline() will reallocate the buffer as needed, we don't need to free it anywhere except right before we exit the function, here. > + > + return head; > +} thanks -- PMM
Hi Peter, How to see the full coverity report? In https://gitlab.com/qemu-project/qemu/-/artifacts, I see only job.log Do you expect to fix these errors for the 9.1 release? Best Regards, Konstantin Kostiuk. On Thu, Jul 25, 2024 at 1:12 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 23 Jul 2024 at 08:03, Konstantin Kostiuk <kkostiuk@redhat.com> > wrote: > > > > From: Dehan Meng <demeng@redhat.com> > > > > The Route information of the Linux VM needs to be used > > by administrators and users when debugging network problems > > and troubleshooting. > > > > Signed-off-by: Dehan Meng <demeng@redhat.com> > > Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com> > > Message-ID: <20240613092802.346246-2-demeng@redhat.com> > > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com> > > Hi; Coverity points out some potential issues with this commit: > > > +static char *hexToIPAddress(const void *hexValue, int is_ipv6) > > +{ > > + if (is_ipv6) { > > + char addr[INET6_ADDRSTRLEN]; > > + struct in6_addr in6; > > + const char *hexStr = (const char *)hexValue; > > + int i; > > + > > + for (i = 0; i < 16; i++) { > > + sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]); > > We don't check the sscanf() return value here. (CID 1558558) > > > + } > > + 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; > > + > > + return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, > byte1); > > + } > > +} > > + > > +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; > > + char *line = NULL; > > + int firstLine; > > + int is_ipv6; > > + int i; > > The handling of the getline() buffer in this function doesn't > seem to be correct (CID 1558559). > > Firstly, the manpage says that to get the initial "allocate me > a buffer", line must be NULL and also n must be 0, but we don't > initialize n here. > > > + for (i = 0; i < 2; i++) { > > + firstLine = 1; > > + is_ipv6 = (i == 1); > > + fp = fopen(routeFiles[i], "r"); > > + if (fp == NULL) { > > + error_setg_errno(errp, errno, "open(\"%s\")", > routeFiles[i]); > > + free(line); > > Here we free() line, but we continue the for() loop. So next > time around the loop (assuming the second fopen succeeds) > we'll pass line to getline() and it will be a non-NULL > pointer to freed memory. > > Is this error case supposed to exit the for() loop entirely > instead of continuing? > > Either way, it shouldn't free(line) here I think. > > > + continue; > > + } > > + > > + while (getline(&line, &n, fp) != -1) { > > + if (firstLine && !is_ipv6) { > > + firstLine = 0; > > + continue; > > + } > > + GuestNetworkRoute *route = NULL; > > + GuestNetworkRoute *networkroute; > > + char Iface[IFNAMSIZ]; > > Our coding style says you shouldn't declare variables in the > middle of a block. Coding style also says variable names are > lowercase with underscores, not CamelCase. (CamelCase is for > typenames.) > > > + 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 (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s", > > + Destination, &DesPrefixlen, Source, > > + &SrcPrefixlen, NextHop, &Metric, &RefCnt, > > + &Use, &Flags, Iface) != 10) { > > + continue; > > + } > > + > > + route = g_new0(GuestNetworkRoute, 1); > > + networkroute = route; > > Why do we have separate "route" and "networkroute" variables > here? As far as I can see they are identical and can be merged. > > > + 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; > > + > > + /* 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) { > > + 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; > > + } > > + > > + QAPI_LIST_APPEND(tail, route); > > + } > > + > > + free(line); > > Similarly here we free(line) but next time around the for() > loop we'll pass it to getline anyway. > > > + fclose(fp); > > + } > > Since getline() will reallocate the buffer as needed, we don't > need to free it anywhere except right before we exit the > function, here. > > > + > > + return head; > > +} > > thanks > -- PMM > >
On Mon, 29 Jul 2024 at 08:40, Konstantin Kostiuk <kkostiuk@redhat.com> wrote: > > Hi Peter, > > How to see the full coverity report? In https://gitlab.com/qemu-project/qemu/-/artifacts, I see only job.log > Do you expect to fix these errors for the 9.1 release? Coverity errors are in https://scan.coverity.com/projects/qemu -- you can ask for an account with the project if you want to see them directly. But I think you have the information you need in this email: the actual coverity issue isn't much more informative. > Do you expect to fix these errors for the 9.1 release? No, I post these emails to inform the people responsible for the original commits about the problem so that they can provide fixes -- after all, it's the original author that knows most about the code and how to test it. thanks -- PMM
On Mon, 29 Jul 2024 at 10:35, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 29 Jul 2024 at 08:40, Konstantin Kostiuk <kkostiuk@redhat.com> wrote: > > > > Hi Peter, > > > > How to see the full coverity report? In https://gitlab.com/qemu-project/qemu/-/artifacts, I see only job.log > > Do you expect to fix these errors for the 9.1 release? > > Coverity errors are in https://scan.coverity.com/projects/qemu > -- you can ask for an account with the project if you want > to see them directly. But I think you have the information > you need in this email: the actual coverity issue isn't > much more informative. > > > Do you expect to fix these errors for the 9.1 release? > > No, I post these emails to inform the people responsible > for the original commits about the problem so that they > can provide fixes -- after all, it's the original author > that knows most about the code and how to test it. Konstantin, are you or Dehan planning to write fixes for these bugs? thanks -- PMM
On Thu, Aug 15, 2024 at 5:18 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 29 Jul 2024 at 10:35, Peter Maydell <peter.maydell@linaro.org> > wrote: > > > > On Mon, 29 Jul 2024 at 08:40, Konstantin Kostiuk <kkostiuk@redhat.com> > wrote: > > > > > > Hi Peter, > > > > > > How to see the full coverity report? In > https://gitlab.com/qemu-project/qemu/-/artifacts, I see only job.log > > > Do you expect to fix these errors for the 9.1 release? > > > > Coverity errors are in https://scan.coverity.com/projects/qemu > > -- you can ask for an account with the project if you want > > to see them directly. But I think you have the information > > you need in this email: the actual coverity issue isn't > > much more informative. > > > > > Do you expect to fix these errors for the 9.1 release? > > > > No, I post these emails to inform the people responsible > > for the original commits about the problem so that they > > can provide fixes -- after all, it's the original author > > that knows most about the code and how to test it. > > Konstantin, are you or Dehan planning to write fixes > for these bugs? > Hi Peter, Yes, we plan to fix these bugs for the 9.2 release. Best Regards, Konstantin Kostiuk. > > thanks > -- PMM > >
diff --git a/qga/commands-linux.c b/qga/commands-linux.c index 89bdcded01..51d5e3d927 100644 --- a/qga/commands-linux.c +++ b/qga/commands-linux.c @@ -28,6 +28,10 @@ #include <libudev.h> #endif +#ifdef HAVE_GETIFADDRS +#include <net/if.h> +#endif + #include <sys/statvfs.h> #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM) @@ -2089,3 +2093,132 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp) fclose(fp); return head; } + +static char *hexToIPAddress(const void *hexValue, int is_ipv6) +{ + if (is_ipv6) { + char addr[INET6_ADDRSTRLEN]; + struct in6_addr in6; + const char *hexStr = (const char *)hexValue; + int i; + + for (i = 0; i < 16; i++) { + sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]); + } + 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; + + return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1); + } +} + +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; + 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"); + if (fp == NULL) { + error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]); + free(line); + continue; + } + + while (getline(&line, &n, fp) != -1) { + if (firstLine && !is_ipv6) { + 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 (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s", + Destination, &DesPrefixlen, Source, + &SrcPrefixlen, NextHop, &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; + + /* 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) { + 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; + } + + QAPI_LIST_APPEND(tail, route); + } + + free(line); + fclose(fp); + } + + return head; +} diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index c763163fcd..495706cf73 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -1851,3 +1851,76 @@ 'returns': ['GuestCpuStats'], 'if': 'CONFIG_LINUX' } + +## +# @GuestNetworkRoute: +# +# Route information, currently, only linux supported. +# +# @iface: The destination network or host's egress network interface in the routing table +# +# @destination: The IP address of the target network or host, The final destination of the packet +# +# @metric: Route metric +# +# @gateway: The IP address of the next hop router +# +# @mask: Subnet Mask (IPv4 only) +# +# @irtt: Initial round-trip delay (not for windows, IPv4 only) +# +# @flags: Route flags (not for windows) +# +# @refcnt: The route's reference count (not for windows) +# +# @use: Route usage count (not for windows) +# +# @window: TCP window size, used for flow control (not for windows, IPv4 only) +# +# @mtu: Data link layer maximum packet size (not for windows) +# +# @desprefixlen: Destination prefix length (for IPv6) +# +# @source: Source IP address (for IPv6) +# +# @srcprefixlen: Source prefix length (for IPv6) +# +# @nexthop: Next hop IP address (for IPv6) +# +# @version: IP version (4 or 6) +# +# Since: 9.1 + +## +{ 'struct': 'GuestNetworkRoute', + 'data': {'iface': 'str', + 'destination': 'str', + 'metric': 'int', + '*gateway': 'str', + '*mask': 'str', + '*irtt': 'int', + '*flags': 'uint64', + '*refcnt': 'int', + '*use': 'int', + '*window': 'int', + '*mtu': 'int', + '*desprefixlen': 'str', + '*source': 'str', + '*srcprefixlen': 'str', + '*nexthop': 'str', + 'version': 'int' + }, + 'if': 'CONFIG_LINUX' } + +## +# @guest-network-get-route: +# +# Retrieve information about route of network. +# Returns: List of route info of guest. +# +# Since: 9.1 +## +{ 'command': 'guest-network-get-route', + 'returns': ['GuestNetworkRoute'], + 'if': 'CONFIG_LINUX' +}