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 <kkost...@redhat.com>
wrote:
>
>
> On Mon, Oct 21, 2024 at 7:45 PM Daniel P. Berrangé <berra...@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 <dem...@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 :|
>>
>>
