| Message ID | 20210610155811.3313927-1-konstantin@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] qga-win: Free GMatchInfo properly | expand |
On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk wrote: > The g_regex_match function creates match_info even if it > returns FALSE. So we should always call g_match_info_free. > A better solution is using g_autoptr for match_info variable. > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > --- > qga/commands-win32.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 300b87c859..785a5cc6b2 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) > continue; > } > for (j = 0; hw_ids[j] != NULL; j++) { > - GMatchInfo *match_info; > + g_autoptr(GMatchInfo) match_info; This should be initialized to NULL otherwise... > GuestDeviceIdPCI *id; > if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) { > continue; this continue will trigger freeing of unintialized memory Essentially all g_auto* variables should be init to NULL at all times, even if it currently looks harmless. > @@ -2511,7 +2511,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) > id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16); > id->device_id = g_ascii_strtoull(device_id, NULL, 16); > > - g_match_info_free(match_info); > break; > } > if (skip) { Regards, Daniel
On 6/10/21 5:58 PM, Kostiantyn Kostiuk wrote: > The g_regex_match function creates match_info even if it > returns FALSE. So we should always call g_match_info_free. > A better solution is using g_autoptr for match_info variable. > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > --- > qga/commands-win32.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Thu, Jun 10, 2021 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk wrote: > > The g_regex_match function creates match_info even if it > > returns FALSE. So we should always call g_match_info_free. > > A better solution is using g_autoptr for match_info variable. > > > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > > --- > > qga/commands-win32.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index 300b87c859..785a5cc6b2 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > **errp) > > continue; > > } > > for (j = 0; hw_ids[j] != NULL; j++) { > > - GMatchInfo *match_info; > > + g_autoptr(GMatchInfo) match_info; > > This should be initialized to NULL otherwise... > > > GuestDeviceIdPCI *id; > > if (!g_regex_match(device_pci_re, hw_ids[j], 0, > &match_info)) { > > continue; > > this continue will trigger freeing of unintialized memory > But we always call match_info, so match_info is always initialized. The g_regex_match function creates match_info even if it returns FALSE. > > Essentially all g_auto* variables should be init to NULL > at all times, even if it currently looks harmless. > > > @@ -2511,7 +2511,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > **errp) > > id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16); > > id->device_id = g_ascii_strtoull(device_id, NULL, 16); > > > > - g_match_info_free(match_info); > > break; > > } > > if (skip) { > > 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 :| > > Best wishes, Kostiantyn Kostiuk
On Thu, Jun 10, 2021 at 07:08:36PM +0300, Konstantin Kostiuk wrote: > On Thu, Jun 10, 2021 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk wrote: > > > The g_regex_match function creates match_info even if it > > > returns FALSE. So we should always call g_match_info_free. > > > A better solution is using g_autoptr for match_info variable. > > > > > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > > > --- > > > qga/commands-win32.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > > index 300b87c859..785a5cc6b2 100644 > > > --- a/qga/commands-win32.c > > > +++ b/qga/commands-win32.c > > > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > > **errp) > > > continue; > > > } > > > for (j = 0; hw_ids[j] != NULL; j++) { > > > - GMatchInfo *match_info; > > > + g_autoptr(GMatchInfo) match_info; > > > > This should be initialized to NULL otherwise... > > > > > GuestDeviceIdPCI *id; > > > if (!g_regex_match(device_pci_re, hw_ids[j], 0, > > &match_info)) { > > > continue; > > > > this continue will trigger freeing of unintialized memory > > > > But we always call match_info, so match_info is always initialized. > The g_regex_match function creates match_info even if it returns FALSE. Opps, yes, you are right. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
CC Michael Roth On Thu, Jun 10, 2021 at 7:14 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jun 10, 2021 at 07:08:36PM +0300, Konstantin Kostiuk wrote: > > On Thu, Jun 10, 2021 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com> > > wrote: > > > > > On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk wrote: > > > > The g_regex_match function creates match_info even if it > > > > returns FALSE. So we should always call g_match_info_free. > > > > A better solution is using g_autoptr for match_info variable. > > > > > > > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > > > > --- > > > > qga/commands-win32.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > > > index 300b87c859..785a5cc6b2 100644 > > > > --- a/qga/commands-win32.c > > > > +++ b/qga/commands-win32.c > > > > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList > *qmp_guest_get_devices(Error > > > **errp) > > > > continue; > > > > } > > > > for (j = 0; hw_ids[j] != NULL; j++) { > > > > - GMatchInfo *match_info; > > > > + g_autoptr(GMatchInfo) match_info; > > > > > > This should be initialized to NULL otherwise... > > > > > > > GuestDeviceIdPCI *id; > > > > if (!g_regex_match(device_pci_re, hw_ids[j], 0, > > > &match_info)) { > > > > continue; > > > > > > this continue will trigger freeing of unintialized memory > > > > > > > But we always call match_info, so match_info is always initialized. > > The g_regex_match function creates match_info even if it returns FALSE. > > Opps, yes, you are right. > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > 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 :| > >
ping On Wed, Jul 14, 2021 at 10:26 AM Konstantin Kostiuk <konstantin@daynix.com> wrote: > CC Michael Roth > > On Thu, Jun 10, 2021 at 7:14 PM Daniel P. Berrangé <berrange@redhat.com> > wrote: > >> On Thu, Jun 10, 2021 at 07:08:36PM +0300, Konstantin Kostiuk wrote: >> > On Thu, Jun 10, 2021 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com >> > >> > wrote: >> > >> > > On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk wrote: >> > > > The g_regex_match function creates match_info even if it >> > > > returns FALSE. So we should always call g_match_info_free. >> > > > A better solution is using g_autoptr for match_info variable. >> > > > >> > > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> >> > > > --- >> > > > qga/commands-win32.c | 3 +-- >> > > > 1 file changed, 1 insertion(+), 2 deletions(-) >> > > > >> > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c >> > > > index 300b87c859..785a5cc6b2 100644 >> > > > --- a/qga/commands-win32.c >> > > > +++ b/qga/commands-win32.c >> > > > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList >> *qmp_guest_get_devices(Error >> > > **errp) >> > > > continue; >> > > > } >> > > > for (j = 0; hw_ids[j] != NULL; j++) { >> > > > - GMatchInfo *match_info; >> > > > + g_autoptr(GMatchInfo) match_info; >> > > >> > > This should be initialized to NULL otherwise... >> > > >> > > > GuestDeviceIdPCI *id; >> > > > if (!g_regex_match(device_pci_re, hw_ids[j], 0, >> > > &match_info)) { >> > > > continue; >> > > >> > > this continue will trigger freeing of unintialized memory >> > > >> > >> > But we always call match_info, so match_info is always initialized. >> > The g_regex_match function creates match_info even if it returns FALSE. >> >> Opps, yes, you are right. >> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >> >> >> 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 :| >> >>
Still candidate for 6.1. On 7/28/21 9:54 AM, Konstantin Kostiuk wrote: > ping > > On Wed, Jul 14, 2021 at 10:26 AM Konstantin Kostiuk > <konstantin@daynix.com <mailto:konstantin@daynix.com>> wrote: > > CC Michael Roth > > On Thu, Jun 10, 2021 at 7:14 PM Daniel P. Berrangé > <berrange@redhat.com <mailto:berrange@redhat.com>> wrote: > > On Thu, Jun 10, 2021 at 07:08:36PM +0300, Konstantin Kostiuk wrote: > > On Thu, Jun 10, 2021 at 7:02 PM Daniel P. Berrangé > <berrange@redhat.com <mailto:berrange@redhat.com>> > > wrote: > > > > > On Thu, Jun 10, 2021 at 06:58:11PM +0300, Kostiantyn Kostiuk > wrote: > > > > The g_regex_match function creates match_info even if it > > > > returns FALSE. So we should always call g_match_info_free. > > > > A better solution is using g_autoptr for match_info variable. > > > > > > > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com > <mailto:konstantin@daynix.com>> > > > > --- > > > > qga/commands-win32.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > > > index 300b87c859..785a5cc6b2 100644 > > > > --- a/qga/commands-win32.c > > > > +++ b/qga/commands-win32.c > > > > @@ -2494,7 +2494,7 @@ GuestDeviceInfoList > *qmp_guest_get_devices(Error > > > **errp) > > > > continue; > > > > } > > > > for (j = 0; hw_ids[j] != NULL; j++) { > > > > - GMatchInfo *match_info; > > > > + g_autoptr(GMatchInfo) match_info; > > > > > > This should be initialized to NULL otherwise... > > > > > > > GuestDeviceIdPCI *id; > > > > if (!g_regex_match(device_pci_re, hw_ids[j], 0, > > > &match_info)) { > > > > continue; > > > > > > this continue will trigger freeing of unintialized memory > > > > > > > But we always call match_info, so match_info is always > initialized. > > The g_regex_match function creates match_info even if it > returns FALSE. > > Opps, yes, you are right. > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com > <mailto:berrange@redhat.com>> > > > Regards, > Daniel > -- > |: https://berrange.com <https://berrange.com> -o- > https://www.flickr.com/photos/dberrange > <https://www.flickr.com/photos/dberrange> :| > |: https://libvirt.org <https://libvirt.org> -o- > https://fstop138.berrange.com <https://fstop138.berrange.com> :| > |: https://entangle-photo.org <https://entangle-photo.org> > -o- https://www.instagram.com/dberrange > <https://www.instagram.com/dberrange> :| >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 300b87c859..785a5cc6b2 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -2494,7 +2494,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) continue; } for (j = 0; hw_ids[j] != NULL; j++) { - GMatchInfo *match_info; + g_autoptr(GMatchInfo) match_info; GuestDeviceIdPCI *id; if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) { continue; @@ -2511,7 +2511,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16); id->device_id = g_ascii_strtoull(device_id, NULL, 16); - g_match_info_free(match_info); break; } if (skip) {
The g_regex_match function creates match_info even if it returns FALSE. So we should always call g_match_info_free. A better solution is using g_autoptr for match_info variable. Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> --- qga/commands-win32.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)