| Message ID | 20241021132839.463255-7-demeng@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/4] qemu-ga: 'Null' check for mandatory parameters | expand |
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
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 :| > >
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 :| >> >>
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;
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(-)