[Libosinfo] [libosinfo PATCH 2/4] media: Don't assume identifiers are null terminated

[Fabiano Fidêncio]

On Thu, Feb 14, 2019 at 10:57 AM Christophe Fergeau <cfergeau at redhat.com>
wrote:

> Hey,
>
> On Wed, Feb 13, 2019 at 08:19:25PM +0100, Fabiano Fidêncio wrote:
> > Identifiers as volume-id, application, publisher, and system are not
> > null terminated and cannot be assumed as so.
> >
> > By assuming those are null terminated strings, libosinfo ends up not
> > counting the last character of a MAX_* string and, consequently, not
> > properly identifying medias that have their identifiers with the MAX_*
> > size.
> >
> > One example is the ubuntu-18.04.1.0-live-server-amd64.iso media, which
> > has as volume-id 'Ubuntu-Server 18.04.1+ LTS amd64'. As the volume-id
> > has exactly 32 characters it's never been matched as when reading the
> > media's volume-id it'd be read as 'Ubuntu-Server 18.04.1+ LTS amd6'.
>
> I don't think we have any test case for that code, do we?
>

We don't have any specific test case for this code. But the whole
test-isodetect relies on this (and that's the way I found out the issue
when adding the new test data).

Do you want a specific test for this or are you fine with the patch 4
covering this case?


>
> > ---
> >  osinfo/osinfo_media.c | 37 +++++++++++++++++++++++--------------
> >  1 file changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/osinfo/osinfo_media.c b/osinfo/osinfo_media.c
> > index 9f77504..eaf67e2 100644
> > --- a/osinfo/osinfo_media.c
> > +++ b/osinfo/osinfo_media.c
> > @@ -137,6 +137,11 @@ struct _CreateFromLocationAsyncData {
> >
> >      gsize offset;
> >      gsize length;
> > +
> > +    gchar *volume;
> > +    gchar *system;
> > +    gchar *application;
> > +    gchar *publisher;
> >  };
> >
> >  static void create_from_location_async_data_free
> > @@ -144,6 +149,10 @@ static void create_from_location_async_data_free
> >  {
> >     g_object_unref(data->file);
> >     g_object_unref(data->res);
> > +   g_free(data->volume);
> > +   g_free(data->system);
> > +   g_free(data->application);
> > +   g_free(data->publisher);
> >
> >     g_slice_free(CreateFromLocationAsyncData, data);
> >  }
> > @@ -809,18 +818,18 @@
> create_from_location_async_data(CreateFromLocationAsyncData *data)
> >                              OSINFO_MEDIA_PROP_URL,
> >                              uri);
> >      g_free(uri);
> > -    if (!is_str_empty(data->pvd.volume))
> > +    if (!is_str_empty(data->volume))
> >          osinfo_entity_set_param(OSINFO_ENTITY(media),
> >                                  OSINFO_MEDIA_PROP_VOLUME_ID,
> > -                                data->pvd.volume);
> > -    if (!is_str_empty(data->pvd.system))
> > +                                data->volume);
> > +    if (!is_str_empty(data->system))
> >          osinfo_entity_set_param(OSINFO_ENTITY(media),
> >                                  OSINFO_MEDIA_PROP_SYSTEM_ID,
> > -                                data->pvd.system);
> > -    if (!is_str_empty(data->pvd.publisher))
> > +                                data->system);
> > +    if (!is_str_empty(data->publisher))
> >          osinfo_entity_set_param(OSINFO_ENTITY(media),
> >                                  OSINFO_MEDIA_PROP_PUBLISHER_ID,
> > -                                data->pvd.publisher);
> > +                                data->publisher);
> >      if (!is_str_empty(data->pvd.application))
> >          osinfo_entity_set_param(OSINFO_ENTITY(media),
> >                                  OSINFO_MEDIA_PROP_APPLICATION_ID,
> > @@ -1159,19 +1168,19 @@ static void on_pvd_read(GObject *source,
> >          return;
> >      }
> >
> > -    data->pvd.volume[MAX_VOLUME - 1] = 0;
> > -    g_strchomp(data->pvd.volume);
> > +    data->volume = g_strndup(data->pvd.volume, MAX_VOLUME);
> > +    g_strchomp(data->volume);
> >
> > -    data->pvd.system[MAX_SYSTEM - 1] = 0;
> > -    g_strchomp(data->pvd.system);
> > +    data->system = g_strndup(data->pvd.system, MAX_SYSTEM);
> > +    g_strchomp(data->system);
> >
> > -    data->pvd.publisher[MAX_PUBLISHER - 1] = 0;
> > -    g_strchomp(data->pvd.publisher);
> > +    data->publisher = g_strndup(data->pvd.publisher, MAX_PUBLISHER);
> > +    g_strchomp(data->publisher);
> >
> > -    data->pvd.application[MAX_APPLICATION - 1] = 0;
> > +    data->application = g_strndup(data->pvd.application,
> MAX_APPLICATION);
> >      g_strchomp(data->pvd.application);
>
> This should be g_strchomp(data->application);
>

Fixed.


>
> Reviewed-by: Christophe Fergeau <cfergeau at redhat.com>
>
> Christophe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libosinfo/attachments/20190214/39680228/attachment.htm>