[PATCH v2] bundle-uri: plug leak in unbundle_from_file()

From

Toon Claes <toon@iotcl.com>

Date

Oct 10, 2024, 09:12 UTC

Message-ID

<20241010091249.1895960-1-toon@iotcl.com>

In-Reply-To

<20240826083052.1542228-1-toon@iotcl.com>

The function `unbundle_from_file()` has two memory leaks:

  - We do not release the `struct bundle_header header` when hitting
    errors because we return early without any cleanup.

  - We do not release the `struct strbuf bundle_ref` at all.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 bundle-uri.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 4b1a2e2937..0df66e2872 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -368,17 +368,23 @@ static int unbundle_from_file(struct repository *r, const char *file)
 	struct strbuf bundle_ref = STRBUF_INIT;
 	size_t bundle_prefix_len;
 
-	if ((bundle_fd = read_bundle_header(file, &header)) < 0)
-		return 1;
+	bundle_fd = read_bundle_header(file, &header);
+	if (bundle_fd < 0) {
+		result = 1;
+		goto cleanup;
+	}
 
 	/*
 	 * Skip the reachability walk here, since we will be adding
 	 * a reachable ref pointing to the new tips, which will reach
 	 * the prerequisite commits.
 	 */
-	if ((result = unbundle(r, &header, bundle_fd, NULL,
-			       VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))
-		return 1;
+	result = unbundle(r, &header, bundle_fd, NULL,
+			  VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));
+	if (result) {
+		result = 1;
+		goto cleanup;
+	}
 
 	/*
 	 * Convert all refs/heads/ from the bundle into refs/bundles/
@@ -407,6 +413,8 @@ static int unbundle_from_file(struct repository *r, const char *file)
 				0, UPDATE_REFS_MSG_ON_ERR);
 	}
 
+cleanup:
+	strbuf_release(&bundle_ref);
 	bundle_header_release(&header);
 	return result;
 }

Range-diff against v1:
1:  f30f393e05 ! 1:  19714d860c bundle-uri: plug leak in unbundle_from_file()
    @@ Metadata
      ## Commit message ##
         bundle-uri: plug leak in unbundle_from_file()
     
    -    When the function returns early, the variable bundle_ref is not released
    -    through strbuf_release().
    +    The function `unbundle_from_file()` has two memory leaks:
     
    -    Fix this leak. And while at it, remove assignments in the conditions of
    -    the "if" statements as suggested in the CodingGuidelines.
    +      - We do not release the `struct bundle_header header` when hitting
    +        errors because we return early without any cleanup.
    +
    +      - We do not release the `struct strbuf bundle_ref` at all.
    +
    +    Plug these leaks by creating a common exit path where both of these
    +    variables are released.
    +
    +    While at it, refactor the code such that the variable assignments do not
    +    happen inside the conditional statement itself according to our coding
    +    style.
     
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
    @@ bundle-uri.c: static int unbundle_from_file(struct repository *r, const char *fi
     -		return 1;
     +	result = unbundle(r, &header, bundle_fd, NULL,
     +			  VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));
    -+	if (result)
    ++	if (result) {
    ++		result = 1;
     +		goto cleanup;
    ++	}
      
      	/*
      	 * Convert all refs/heads/ from the bundle into refs/bundles/

-- 
2.46.0.46.g406f326d27