From 8d926c3e36f5aecf844b545b74291a51111dfc47 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 4 May 2012 10:04:32 -0400 Subject: [PATCH] core: Add valgrind framework, plug various memory leaks --- src/libostree/ostree-core.c | 18 +-- src/libostree/ostree-repo-file.c | 7 +- src/libostree/ostree-repo.c | 14 +-- src/ostree/ostree-pull.c | 8 +- src/ostree/ot-builtin-commit.c | 3 +- src/ostree/ot-builtin-diff.c | 4 + src/ostree/ot-builtin-pack.c | 2 +- src/ostree/ot-builtin-pull-local.c | 4 + tests/libtest.sh | 14 ++- tests/ostree-valgrind.supp | 191 +++++++++++++++++++++++++++++ tests/t0000-basic.sh | 12 +- tests/t0001-archive.sh | 6 +- tests/t0010-pull.sh | 16 +-- 13 files changed, 252 insertions(+), 47 deletions(-) create mode 100644 tests/ostree-valgrind.supp diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index 72906ffe..1f352a7c 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -441,18 +441,18 @@ ostree_content_stream_parse (GInputStream *input, if (ret_file_info) g_file_info_set_size (ret_file_info, input_length - archive_header_size - 8); - if (g_file_info_get_file_type (ret_file_info) != G_FILE_TYPE_REGULAR) + if (g_file_info_get_file_type (ret_file_info) == G_FILE_TYPE_REGULAR + && out_input) { - g_clear_object (&ret_input); + /* Give the input stream at its current position as return value; + * assuming the caller doesn't seek, this should be fine. We might + * want to wrap it though in a non-seekable stream. + **/ + ret_input = g_object_ref (input); } ret = TRUE; - /* Give the input stream at its current position as return value; - * assuming the caller doesn't seek, this should be fine. We might - * want to wrap it though in a non-seekable stream. - **/ - g_object_ref (input); - ot_transfer_out_value (out_input, &input); + ot_transfer_out_value (out_input, &ret_input); ot_transfer_out_value (out_file_info, &ret_file_info); ot_transfer_out_value (out_xattrs, &ret_xattrs); out: @@ -1835,6 +1835,7 @@ ostree_validate_structureof_pack_superindex (GVariant *superindex, goto out; } csum_v = NULL; + bloom = NULL; g_variant_get_child (superindex, 3, "a(ayay)", &content_iter); while (g_variant_iter_loop (content_iter, "(@ay@ay)", @@ -1844,6 +1845,7 @@ ostree_validate_structureof_pack_superindex (GVariant *superindex, goto out; } csum_v = NULL; + bloom = NULL; ret = TRUE; out: diff --git a/src/libostree/ostree-repo-file.c b/src/libostree/ostree-repo-file.c index 005f59e6..dd824238 100644 --- a/src/libostree/ostree-repo-file.c +++ b/src/libostree/ostree-repo-file.c @@ -65,6 +65,7 @@ ostree_repo_file_finalize (GObject *object) g_free (self->tree_contents_checksum); g_free (self->tree_metadata_checksum); g_free (self->commit); + g_clear_error (&self->commit_resolve_error); g_free (self->name); G_OBJECT_CLASS (ostree_repo_file_parent_class)->finalize (object); @@ -400,6 +401,8 @@ ostree_repo_file_get_checksum (OstreeRepoFile *self) self->cached_file_checksum = ostree_checksum_from_bytes_v (csum_bytes); + g_variant_unref (csum_bytes); + return self->cached_file_checksum; } @@ -721,7 +724,7 @@ bsearch_in_file_variant (GVariant *variant, imin = 0; while (imax >= imin) { - GVariant *child; + ot_lvariant GVariant *child = NULL; const char *cur; int cmp; @@ -741,11 +744,9 @@ bsearch_in_file_variant (GVariant *variant, } else { - ot_clear_gvariant (&child); *out_pos = imid; return TRUE; } - ot_clear_gvariant (&child); } *out_pos = imid; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 4fd7f6d7..011fde59 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -265,13 +265,12 @@ parse_rev_file (OstreeRepo *self, if (g_str_has_prefix (rev, "ref: ")) { - GFile *ref; + ot_lobj GFile *ref = NULL; char *ref_sha256; gboolean subret; ref = g_file_resolve_relative_path (priv->local_heads_dir, rev + 5); subret = parse_rev_file (self, ref, &ref_sha256, error); - g_clear_object (&ref); if (!subret) { @@ -3025,7 +3024,7 @@ list_loose_object_dir (OstreeRepo *self, value = g_variant_new ("(b@as)", TRUE, g_variant_new_strv (NULL, 0)); /* transfer ownership */ - g_hash_table_replace (inout_objects, g_variant_ref_sink (key), + g_hash_table_replace (inout_objects, key, g_variant_ref_sink (value)); } loop_next: @@ -3870,7 +3869,7 @@ checkout_one_file (OstreeRepo *self, if (link_cache) { - ot_lobj GFile *parent; + ot_lobj GFile *parent = NULL; g_assert (possible_loose_path); parent = g_file_get_parent (possible_loose_path); @@ -3907,8 +3906,6 @@ ostree_repo_checkout_tree (OstreeRepo *self, ot_lobj GFileInfo *file_info = NULL; ot_lvariant GVariant *xattrs = NULL; ot_lobj GFileEnumerator *dir_enum = NULL; - ot_lobj GFile *src_child = NULL; - ot_lobj GFile *dest_path = NULL; if (!ostree_repo_file_get_xattrs (source, &xattrs, NULL, error)) goto out; @@ -3931,14 +3928,13 @@ ostree_repo_checkout_tree (OstreeRepo *self, { const char *name; guint32 type; + ot_lobj GFile *dest_path = NULL; + ot_lobj GFile *src_child = NULL; name = g_file_info_get_attribute_byte_string (file_info, "standard::name"); type = g_file_info_get_attribute_uint32 (file_info, "standard::type"); - g_clear_object (&dest_path); dest_path = g_file_get_child (destination, name); - - g_clear_object (&src_child); src_child = g_file_get_child ((GFile*)source, name); if (type == G_FILE_TYPE_DIRECTORY) diff --git a/src/ostree/ostree-pull.c b/src/ostree/ostree-pull.c index 71f8305e..71b53f76 100644 --- a/src/ostree/ostree-pull.c +++ b/src/ostree/ostree-pull.c @@ -280,6 +280,7 @@ fetch_one_pack_file (OtPullData *pull_data, goto out; } + g_clear_object (&ret_cached_path); if (!ostree_repo_get_cached_remote_pack_data (pull_data->repo, pull_data->remote_name, pack_checksum, is_meta, &ret_cached_path, cancellable, error)) @@ -1038,6 +1039,7 @@ ostree_builtin_pull (int argc, char **argv, GFile *repo_path, GError **error) gpointer key, value; int i; GCancellable *cancellable = NULL; + ot_lfree char *tmpstr = NULL; ot_lobj OstreeRepo *repo = NULL; ot_lfree char *path = NULL; ot_lfree char *baseurl = NULL; @@ -1081,8 +1083,8 @@ ostree_builtin_pull (int argc, char **argv, GFile *repo_path, GError **error) config = ostree_repo_get_config (repo); - key = g_strdup_printf ("remote \"%s\"", pull_data->remote_name); - baseurl = g_key_file_get_string (config, key, "url", error); + tmpstr = g_strdup_printf ("remote \"%s\"", pull_data->remote_name); + baseurl = g_key_file_get_string (config, tmpstr, "url", error); if (!baseurl) goto out; pull_data->base_uri = soup_uri_new (baseurl); @@ -1248,8 +1250,10 @@ ostree_builtin_pull (int argc, char **argv, GFile *repo_path, GError **error) if (context) g_option_context_free (context); g_clear_object (&pull_data->session); + g_free (pull_data->remote_name); if (pull_data->base_uri) soup_uri_free (pull_data->base_uri); + ot_clear_hashtable (&pull_data->file_checksums_to_fetch); ot_clear_ptrarray (&pull_data->cached_meta_pack_indexes); ot_clear_ptrarray (&pull_data->cached_data_pack_indexes); if (summary_uri) diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c index 689c4f68..add7aa2c 100644 --- a/src/ostree/ot-builtin-commit.c +++ b/src/ostree/ot-builtin-commit.c @@ -188,7 +188,6 @@ ostree_builtin_commit (int argc, char **argv, GFile *repo_path, GError **error) NULL, error); if (!metadata) goto out; - g_variant_ref_sink (metadata); } else if (metadata_bin_path) { @@ -210,7 +209,7 @@ ostree_builtin_commit (int argc, char **argv, GFile *repo_path, GError **error) { const char *s; const char *eq; - char *key; + ot_lfree char *key = NULL; s = *iter; diff --git a/src/ostree/ot-builtin-diff.c b/src/ostree/ot-builtin-diff.c index 91f7933a..335096a7 100644 --- a/src/ostree/ot-builtin-diff.c +++ b/src/ostree/ot-builtin-diff.c @@ -362,6 +362,7 @@ diff_dirs (GFile *a, if (!dir_enum) goto out; + g_clear_object (&child_b_info); while ((child_b_info = g_file_enumerator_next_file (dir_enum, cancellable, &temp_error)) != NULL) { const char *name; @@ -397,6 +398,7 @@ diff_dirs (GFile *a, goto out; } } + g_clear_object (&child_b_info); } if (temp_error != NULL) { @@ -498,5 +500,7 @@ ostree_builtin_diff (int argc, char **argv, GFile *repo_path, GError **error) ret = TRUE; out: + if (context) + g_option_context_free (context); return ret; } diff --git a/src/ostree/ot-builtin-pack.c b/src/ostree/ot-builtin-pack.c index cacb8762..2a62dbfd 100644 --- a/src/ostree/ot-builtin-pack.c +++ b/src/ostree/ot-builtin-pack.c @@ -737,7 +737,7 @@ do_stats_gather_loose (OtRepackData *data, OstreeObjectType objtype; gboolean is_loose; gboolean is_packed; - GVariant *pack_array; + ot_lvariant GVariant *pack_array = NULL; ostree_object_name_deserialize (serialized_key, &checksum, &objtype); diff --git a/src/ostree/ot-builtin-pull-local.c b/src/ostree/ot-builtin-pull-local.c index eb3f0f34..7bdd72d7 100644 --- a/src/ostree/ot-builtin-pull-local.c +++ b/src/ostree/ot-builtin-pull-local.c @@ -206,6 +206,10 @@ ostree_builtin_pull_local (int argc, char **argv, GFile *repo_path, GError **err ret = TRUE; out: + if (data.src_repo) + g_object_unref (data.src_repo); + if (data.dest_repo) + g_object_unref (data.dest_repo); if (context) g_option_context_free (context); return ret; diff --git a/tests/libtest.sh b/tests/libtest.sh index b6e57a1e..691e7c34 100644 --- a/tests/libtest.sh +++ b/tests/libtest.sh @@ -32,6 +32,10 @@ if test -n "${OT_TESTS_DEBUG}"; then set -x fi +if test -n "$OT_TESTS_VALGRIND"; then + CMD_PREFIX="env G_SLICE=always-malloc valgrind -q --leak-check=full --num-callers=30 --suppressions=${SRCDIR}/ostree-valgrind.supp" +fi + die () { if test -z "$OT_TESTS_SAVE_TEMPS"; then test -f "$test_tmpdir/.test$$" && rm -rf "$test_tmpdir" @@ -70,7 +74,7 @@ setup_test_repository () { mkdir repo cd repo ot_repo="--repo=`pwd`" - export OSTREE="ostree ${ot_repo}" + export OSTREE="${CMD_PREFIX} ostree ${ot_repo}" if test "$mode" = "archive"; then $OSTREE init --archive else @@ -110,20 +114,20 @@ setup_fake_remote_repo1() { mkdir ostree-srv cd ostree-srv mkdir gnomerepo - ostree --repo=gnomerepo init --archive + ${CMD_PREFIX} ostree --repo=gnomerepo init --archive mkdir gnomerepo-files cd gnomerepo-files echo first > firstfile mkdir baz echo moo > baz/cow echo alien > baz/saucer - ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "A remote commit" -m "Some Commit body" + ${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "A remote commit" -m "Some Commit body" mkdir baz/deeper - ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "Add deeper" + ${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "Add deeper" echo hi > baz/deeper/ohyeah mkdir baz/another/ echo x > baz/another/y - ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "The rest" + ${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main -s "The rest" cd .. rm -rf gnomerepo-files diff --git a/tests/ostree-valgrind.supp b/tests/ostree-valgrind.supp new file mode 100644 index 00000000..ce22aa78 --- /dev/null +++ b/tests/ostree-valgrind.supp @@ -0,0 +1,191 @@ +{ + g_type_init_with_debug_flags calloc + Memcheck:Leak + fun:calloc + ... + fun:g_type_init_with_debug_flags + ... +} + +{ + g_type_add_interface_static malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_add_interface_static + ... +} + +{ + g_type_add_interface_dynamic malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_add_interface_dynamic + ... +} + +{ + g_type_class_ref malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_class_ref + ... +} + +{ + g_type_register_dynamic malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_register_dynamic + ... +} + +{ + g_type_init_with_debug_flags malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_init_with_debug_flags + ... +} + +{ + g_type_init_with_debug_flags realloc + Memcheck:Leak + fun:realloc + ... + fun:g_type_init_with_debug_flags + ... +} + +{ + g_test_add_vtable malloc + Memcheck:Leak + fun:malloc + ... + fun:g_test_add_vtable + ... +} + +{ + g_test_init + Memcheck:Leak + fun:malloc + ... + fun:g_test_init + ... +} + +{ + g_type_register_static malloc + Memcheck:Leak + fun:malloc + ... + fun:g_type_register_static + ... +} + +{ + g_type_register_static realloc + Memcheck:Leak + fun:realloc + ... + fun:g_type_register_static + ... +} + +{ + g_type_register_fundamental never freed + Memcheck:Leak + fun:malloc + ... + fun:g_type_register_fundamental + ... +} + +{ + g_type_class_ref never finalized + Memcheck:Leak + fun:calloc + fun:g_malloc0 + fun:g_type_class_ref + ... +} + +{ + DBusGValue qdata + Memcheck:Leak + fun:realloc + fun:g_realloc + fun:g_type_set_qdata + fun:_dbus_g_value_types_init + ... +} + +{ + gettext conditional jump + Memcheck:Cond + fun:__GI___strcasecmp_l + fun:__gconv_open + fun:_nl_find_msg + fun:__dcigettext + ... +} + +{ + gettext uninitialized value + Memcheck:Value8 + fun:__GI___strcasecmp_l + fun:__gconv_open + fun:_nl_find_msg + fun:__dcigettext + ... +} + +{ + font config invalid reads + Memcheck:Addr4 + ... + fun:FcConfigParseAndLoad + ... +} + +{ + dynamic loader conditional jump + Memcheck:Cond + fun:index + fun:expand_dynamic_string_token + fun:_dl_map_object + fun:map_doit + fun:_dl_catch_error + fun:do_preload + fun:dl_main + ... +} + +{ + g_vfs_get_local + Memcheck:Leak + ... + fun:g_vfs_get_local + ... +} + +{ + _g_io_modules_ensure_loaded + Memcheck:Leak + ... + fun:_g_io_modules_ensure_loaded + ... +} + +{ + _g_io_module_get_default + Memcheck:Leak + ... + fun:_g_io_module_get_default + ... +} diff --git a/tests/t0000-basic.sh b/tests/t0000-basic.sh index d59d2315..379a8e92 100755 --- a/tests/t0000-basic.sh +++ b/tests/t0000-basic.sh @@ -131,12 +131,12 @@ echo "ok metadata content" cd ${test_tmpdir} mkdir repo2 -ostree --repo=repo2 init -ostree --repo=repo2 pull-local repo +${CMD_PREFIX} ostree --repo=repo2 init +${CMD_PREFIX} ostree --repo=repo2 pull-local repo echo "ok pull-local" cd ${test_tmpdir} -ostree --repo=repo2 checkout test2 test2-checkout-from-local-clone +${CMD_PREFIX} ostree --repo=repo2 checkout test2 test2-checkout-from-local-clone cd test2-checkout-from-local-clone assert_file_has_content yet/another/tree/green 'leaf' echo "ok local clone checkout" @@ -223,9 +223,9 @@ echo "ok checkout link cache" cd ${test_tmpdir} rm -rf shadow-repo mkdir shadow-repo -ostree --repo=shadow-repo init -ostree --repo=shadow-repo config set core.parent $(pwd)/repo +${CMD_PREFIX} ostree --repo=shadow-repo init +${CMD_PREFIX} ostree --repo=shadow-repo config set core.parent $(pwd)/repo rm -rf test2-checkout parent_rev_test2=$(ostree --repo=repo rev-parse test2) -ostree --repo=shadow-repo checkout "${parent_rev_test2}" test2-checkout +${CMD_PREFIX} ostree --repo=shadow-repo checkout "${parent_rev_test2}" test2-checkout echo "ok checkout from shadow repo" diff --git a/tests/t0001-archive.sh b/tests/t0001-archive.sh index 2b2c1d22..496b6c71 100755 --- a/tests/t0001-archive.sh +++ b/tests/t0001-archive.sh @@ -38,12 +38,12 @@ echo "ok content" cd ${test_tmpdir} mkdir repo2 -ostree --repo=repo2 init -ostree --repo=repo2 pull-local repo +${CMD_PREFIX} ostree --repo=repo2 init +${CMD_PREFIX} ostree --repo=repo2 pull-local repo echo "ok local clone" cd ${test_tmpdir} -ostree --repo=repo2 checkout test2 test2-checkout-from-local-clone +${CMD_PREFIX} ostree --repo=repo2 checkout test2 test2-checkout-from-local-clone cd test2-checkout-from-local-clone assert_file_has_content baz/cow moo echo "ok local clone checkout" diff --git a/tests/t0010-pull.sh b/tests/t0010-pull.sh index 5f58d117..57416c60 100755 --- a/tests/t0010-pull.sh +++ b/tests/t0010-pull.sh @@ -26,10 +26,10 @@ echo '1..4' setup_fake_remote_repo1 cd ${test_tmpdir} mkdir repo -ostree --repo=repo init -ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo -ostree-pull --repo=repo origin main -ostree --repo=repo fsck +${CMD_PREFIX} ostree --repo=repo init +${CMD_PREFIX} ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo +${CMD_PREFIX} ostree-pull --repo=repo origin main +${CMD_PREFIX} ostree --repo=repo fsck echo "ok pull" cd ${test_tmpdir} @@ -43,10 +43,10 @@ cd ${test_tmpdir} ostree --repo=$(pwd)/ostree-srv/gnomerepo pack rm -rf repo mkdir repo -ostree --repo=repo init -ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo -ostree-pull --repo=repo origin main -ostree --repo=repo fsck +${CMD_PREFIX} ostree --repo=repo init +${CMD_PREFIX} ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo +${CMD_PREFIX} ostree-pull --repo=repo origin main +${CMD_PREFIX} ostree --repo=repo fsck echo "ok pull packed" cd ${test_tmpdir}