Re: [PATCH 8/8] db: Fix valgrind issues with language detection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 05, 2015 at 05:20:46PM +0100, Christophe Fergeau wrote:
> match_languages() returns a list containing a single string, but given
> the way language_code_from_raw() works, this string may actually point
> to already freed memory by the time match_languages() ends.
> Subsequent uses of this list will cause accesses to free'd memory.
> 
> This commit changes match_languages() so that it uses the string itself
> (before it's freed) rather than letting the caller do this.
> 
> This fixes:
> ==28623== Invalid read of size 1
> ==28623==    at 0x4C2BC22: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==28623==    by 0x6A6B1F2: g_strdup (gstrfuncs.c:362)
> ==28623==    by 0x553FCA2: osinfo_entity_add_param (osinfo_entity.c:253)
> ==28623==    by 0x5554BD8: osinfo_media_set_languages (osinfo_media.c:1256)
> ==28623==    by 0x555A78A: fill_media (osinfo_db.c:641)
> ==28623==    by 0x555ABED: osinfo_db_identify_media (osinfo_db.c:709)
> ==28623==    by 0x4030AA: test_one (test-isodetect.c:346)
> ==28623==    by 0x40339D: test_windows (test-isodetect.c:395)
> ==28623==    by 0x532B78A: tcase_run_tfun_nofork.isra.9 (check_run.c:390)
> ==28623==    by 0x532BB7C: srunner_iterate_tcase_tfuns (check_run.c:231)
> ==28623==    by 0x532BB7C: srunner_run_tcase (check_run.c:373)
> ==28623==    by 0x532BB7C: srunner_iterate_suites (check_run.c:195)
> ==28623==    by 0x532BB7C: srunner_run (check_run.c:782)
> ==28623==    by 0x403A1B: main (test-isodetect.c:500)
> ==28623==  Address 0x89914b0 is 0 bytes inside a block of size 4 free'd
> ==28623==    at 0x4C29D6A: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==28623==    by 0x6A525ED: g_free (gmem.c:189)
> ==28623==    by 0x55586A6: match_languages (osinfo_db.c:117)
> ==28623==    by 0x555A76C: fill_media (osinfo_db.c:639)
> ==28623==    by 0x555ABED: osinfo_db_identify_media (osinfo_db.c:709)
> ==28623==    by 0x4030AA: test_one (test-isodetect.c:346)
> ==28623==    by 0x40339D: test_windows (test-isodetect.c:395)
> ==28623==    by 0x532B78A: tcase_run_tfun_nofork.isra.9 (check_run.c:390)
> ==28623==    by 0x532BB7C: srunner_iterate_tcase_tfuns (check_run.c:231)
> ==28623==    by 0x532BB7C: srunner_run_tcase (check_run.c:373)
> ==28623==    by 0x532BB7C: srunner_iterate_suites (check_run.c:195)
> ==28623==    by 0x532BB7C: srunner_run (check_run.c:782)
> ==28623==    by 0x403A1B: main (test-isodetect.c:500)
> ==28623==  Block was alloc'd at
> ==28623==    at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==28623==    by 0x6A524D8: g_malloc (gmem.c:94)
> ==28623==    by 0x6A6B277: g_strndup (gstrfuncs.c:425)
> ==28623==    by 0x6A5EC90: g_match_info_fetch (gregex.c:1005)
> ==28623==    by 0x55583DF: get_raw_lang (osinfo_db.c:57)
> ==28623==    by 0x5558672: match_languages (osinfo_db.c:113)
> ==28623==    by 0x555A76C: fill_media (osinfo_db.c:639)
> ==28623==    by 0x555ABED: osinfo_db_identify_media (osinfo_db.c:709)
> ==28623==    by 0x4030AA: test_one (test-isodetect.c:346)
> ==28623==    by 0x40339D: test_windows (test-isodetect.c:395)
> ==28623==    by 0x532B78A: tcase_run_tfun_nofork.isra.9 (check_run.c:390)
> ==28623==    by 0x532BB7C: srunner_iterate_tcase_tfuns (check_run.c:231)
> ==28623==    by 0x532BB7C: srunner_run_tcase (check_run.c:373)
> ==28623==    by 0x532BB7C: srunner_iterate_suites (check_run.c:195)
> ==28623==    by 0x532BB7C: srunner_run (check_run.c:782)
> ==28623==    by 0x403A1B: main (test-isodetect.c:500)
> ---
>  osinfo/osinfo_db.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/osinfo/osinfo_db.c b/osinfo/osinfo_db.c
> index 4eeb440..82751a6 100644
> --- a/osinfo/osinfo_db.c
> +++ b/osinfo/osinfo_db.c
> @@ -80,8 +80,8 @@ static const char *language_code_from_raw(OsinfoDatamap *lang_map,
>      return lang;
>  }
>  
> -static GList *match_languages(OsinfoDb *db, OsinfoMedia *media,
> -                              OsinfoMedia *db_media)
> +static void set_languages_for_media(OsinfoDb *db, OsinfoMedia *media,
> +                                    OsinfoMedia *db_media)
>  {
>      const gchar *volume_id;
>      const gchar *regex;
> @@ -89,18 +89,19 @@ static GList *match_languages(OsinfoDb *db, OsinfoMedia *media,
>      OsinfoDatamap *lang_map;
>      gchar *raw_lang;
>      GList *languages;
> +    const char *lang;
>  
> -    g_return_val_if_fail(OSINFO_IS_MEDIA(media), NULL);
> -    g_return_val_if_fail(OSINFO_IS_MEDIA(db_media), NULL);
> +    g_return_if_fail(OSINFO_IS_MEDIA(media));
> +    g_return_if_fail(OSINFO_IS_MEDIA(db_media));
>  
>      regex = osinfo_entity_get_param_value(OSINFO_ENTITY(db_media),
>                                            OSINFO_MEDIA_PROP_LANG_REGEX);
>      if (regex == NULL)
> -        return NULL;
> +        return;
>  
>      volume_id = osinfo_media_get_volume_id(media);
>      if (volume_id == NULL)
> -        return NULL;
> +        return;
>  
>      lang_map_id = osinfo_entity_get_param_value(OSINFO_ENTITY(db_media),
>                                                  OSINFO_MEDIA_PROP_LANG_MAP);
> @@ -111,14 +112,14 @@ static GList *match_languages(OsinfoDb *db, OsinfoMedia *media,
>      }
>  
>      raw_lang = get_raw_lang(volume_id, regex);
> -
> -    languages = g_list_append(NULL,
> -                              (gpointer)language_code_from_raw(lang_map, raw_lang));
> +    lang = language_code_from_raw(lang_map, raw_lang);
> +    languages = g_list_append(NULL, (gpointer)lang);
> +    osinfo_media_set_languages(media, languages);
> +    g_list_free(languages);
>      g_free(raw_lang);
> -
> -    return languages;
>  }
>  
> +
>  /**
>   * SECTION:osinfo_db
>   * @short_description: Database of all entities
> @@ -625,7 +626,6 @@ static void fill_media(OsinfoDb *db, OsinfoMedia *media,
>                          OsinfoMedia *matched_media,
>                          OsinfoOs *os)
>  {
> -    GList *languages;
>      gboolean is_installer;
>      gboolean is_live;
>      gint reboots;
> @@ -636,10 +636,7 @@ static void fill_media(OsinfoDb *db, OsinfoMedia *media,
>      const gchar *url;
>      GList *variants, *node;
>  
> -    languages = match_languages(db, media, matched_media);
> -    if (languages != NULL)
> -        osinfo_media_set_languages(media, languages);
> -    g_list_free(languages);
> +    set_languages_for_media(db, media, matched_media);
>  
>      id = osinfo_entity_get_id(OSINFO_ENTITY(matched_media));
>      g_object_set(G_OBJECT(media), "id", id, NULL);

ACK


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux