From 862e6ecdcc58f025696b1394adfc0fcf7322df23 Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Wed, 11 May 2016 11:04:04 +0200 Subject: [PATCH] libostree: Variant-related leak plugs and fixes This tries to avoid leaking GVariantBuilders and GVariants in some situations. The leaks were usually happening when some error occurred or because of unclear variant ownership situation. The former is mostly about making sure that g_variant_builder_clear is called on builders that didn't finish their variant building process. The latter is surely more work - sometimes the result of g_variant_builder_end() should not be passed directly to a function, but rather stored in a g_autoptr(GVariant), sunk and then passed to a function. IMO, with an advent of g_autoptr, GVariants should be always sunk instead of relying on some receiver function sinking it. This would make an easy-to-follow policy of always sinking your variants. Functions could then assume that the passed variant is already sunk. These leaks are still happenning in commands, but they are less harmful, since that code will not be used by some daemon as a library routine. Closes: #291 Approved by: cgwalters --- src/libostree/ostree-repo-commit.c | 2 +- src/libostree/ostree-repo-libarchive.c | 4 +++- src/libostree/ostree-repo-pull.c | 5 ++++- .../ostree-repo-static-delta-compilation.c | 14 ++++++++------ src/libostree/ostree-repo.c | 16 +++++++++------- 5 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 170b7b87..f384624c 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2384,7 +2384,7 @@ get_modified_xattrs (OstreeRepo *self, if (label) { - GVariantBuilder *builder; + g_autoptr(GVariantBuilder) builder = NULL; /* ret_xattrs may be NULL */ builder = ot_util_variant_builder_from_variant (ret_xattrs, diff --git a/src/libostree/ostree-repo-libarchive.c b/src/libostree/ostree-repo-libarchive.c index 57da41d4..0d62124d 100644 --- a/src/libostree/ostree-repo-libarchive.c +++ b/src/libostree/ostree-repo-libarchive.c @@ -331,6 +331,7 @@ aic_ensure_parent_dir_with_file_info (OstreeRepoArchiveImportContext *ctx, { const char *name = glnx_basename (fullpath); g_auto(GVariantBuilder) xattrs_builder; + g_autoptr(GVariant) xattrs = NULL; /* is this the root directory itself? transform into empty string */ if (name[0] == '/' && name[1] == '\0') @@ -343,8 +344,9 @@ aic_ensure_parent_dir_with_file_info (OstreeRepoArchiveImportContext *ctx, DEFAULT_DIRMODE, cancellable, error)) return FALSE; + xattrs = g_variant_ref_sink (g_variant_builder_end (&xattrs_builder)); return mtree_ensure_dir_with_meta (ctx->repo, parent, name, file_info, - g_variant_builder_end (&xattrs_builder), + xattrs, FALSE /* error_if_exist */, out_dir, cancellable, error); } diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 708df5b6..619657c7 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1759,6 +1759,8 @@ ostree_repo_pull_one_dir (OstreeRepo *self, GError **error) { GVariantBuilder builder; + g_autoptr(GVariant) options = NULL; + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}")); if (dir_to_pull) @@ -1770,7 +1772,8 @@ ostree_repo_pull_one_dir (OstreeRepo *self, g_variant_builder_add (&builder, "{s@v}", "refs", g_variant_new_variant (g_variant_new_strv ((const char *const*) refs_to_fetch, -1))); - return ostree_repo_pull_with_options (self, remote_name, g_variant_builder_end (&builder), + options = g_variant_ref_sink (g_variant_builder_end (&builder)); + return ostree_repo_pull_with_options (self, remote_name, options, progress, cancellable, error); } diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index 2a28e7db..218bbcc8 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -1253,7 +1253,7 @@ ostree_repo_static_delta_generate (OstreeRepo *self, guint min_fallback_size; guint max_bsdiff_size; guint max_chunk_size; - GVariantBuilder metadata_builder; + g_auto(GVariantBuilder) metadata_builder = {0,}; DeltaOpts delta_opts = DELTAOPT_FLAG_NONE; guint64 total_compressed_size = 0; guint64 total_uncompressed_size = 0; @@ -1384,16 +1384,18 @@ ostree_repo_static_delta_generate (OstreeRepo *self, g_autoptr(GVariant) delta_part_content = NULL; g_autoptr(GVariant) delta_part = NULL; g_autoptr(GVariant) delta_part_header = NULL; - GVariantBuilder *mode_builder = g_variant_builder_new (G_VARIANT_TYPE ("a(uuu)")); - GVariantBuilder *xattr_builder = g_variant_builder_new (G_VARIANT_TYPE ("aa(ayay)")); + g_auto(GVariantBuilder) mode_builder = {0,}; + g_auto(GVariantBuilder) xattr_builder = {0,}; guint8 compression_type_char; + g_variant_builder_init (&mode_builder, G_VARIANT_TYPE ("a(uuu)")); + g_variant_builder_init (&xattr_builder, G_VARIANT_TYPE ("aa(ayay)")); { guint j; for (j = 0; j < part_builder->modes->len; j++) - g_variant_builder_add_value (mode_builder, part_builder->modes->pdata[j]); + g_variant_builder_add_value (&mode_builder, part_builder->modes->pdata[j]); for (j = 0; j < part_builder->xattrs->len; j++) - g_variant_builder_add_value (xattr_builder, part_builder->xattrs->pdata[j]); + g_variant_builder_add_value (&xattr_builder, part_builder->xattrs->pdata[j]); } payload_b = g_string_free_to_bytes (part_builder->payload); @@ -1403,7 +1405,7 @@ ostree_repo_static_delta_generate (OstreeRepo *self, part_builder->operations = NULL; /* FIXME - avoid duplicating memory here */ delta_part_content = g_variant_new ("(a(uuu)aa(ayay)@ay@ay)", - mode_builder, xattr_builder, + &mode_builder, &xattr_builder, ot_gvariant_new_ay_bytes (payload_b), ot_gvariant_new_ay_bytes (operations_b)); g_variant_ref_sink (delta_part_content); diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 42813e9c..91f9615e 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -3510,13 +3510,16 @@ ostree_repo_delete_object (OstreeRepo *self, if (tombstone_commits) { - g_autoptr(GVariantBuilder) builder = NULL; - builder = g_variant_builder_new (G_VARIANT_TYPE ("a{sv}")); - g_variant_builder_add (builder, "{sv}", "commit", g_variant_new_bytestring (sha256)); + g_auto(GVariantBuilder) builder = {0,}; + g_autoptr(GVariant) variant = NULL; + + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}")); + g_variant_builder_add (&builder, "{sv}", "commit", g_variant_new_bytestring (sha256)); + variant = g_variant_ref_sink (g_variant_builder_end (&builder)); if (!ostree_repo_write_metadata_trusted (self, OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT, sha256, - g_variant_builder_end (builder), + variant, cancellable, error)) goto out; @@ -4286,7 +4289,6 @@ ostree_repo_append_gpg_signature (OstreeRepo *self, gboolean ret = FALSE; g_autoptr(GVariant) metadata = NULL; g_autoptr(GVariant) new_metadata = NULL; - g_autoptr(GVariantBuilder) builder = NULL; if (!ostree_repo_read_commit_detached_metadata (self, commit_checksum, @@ -4954,7 +4956,7 @@ ostree_repo_regenerate_summary (OstreeRepo *self, g_autoptr(GVariant) summary = NULL; GList *ordered_keys = NULL; GList *iter = NULL; - GVariantDict additional_metadata_builder; + g_auto(GVariantDict) additional_metadata_builder = {0,}; if (!ostree_repo_list_refs (self, NULL, &refs, cancellable, error)) goto out; @@ -4987,7 +4989,7 @@ ostree_repo_regenerate_summary (OstreeRepo *self, { guint i; g_autoptr(GPtrArray) delta_names = NULL; - GVariantDict deltas_builder; + g_auto(GVariantDict) deltas_builder = {0,}; g_autoptr(GVariant) deltas = NULL; if (!ostree_repo_list_static_delta_names (self, &delta_names, cancellable, error))