[PATCH 1/2] refs/files-backend: duplicate...

[PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

From: Martin Ågren <hidden>
Date: 2017-08-25 18:49:49

files_transaction_prepare() and the functions it calls add strings to a
string list without duplicating them, i.e., we keep the original raw
pointers we were given. That is "ok", since we keep them only for a
short-enough time, but we end up leaking some of them.

Switch to duplicating the strings, so that affected_refnames does not
leak memory. The original strings might still leak, but at least that
can now be addressed without worrying about these pointers.

No-one takes any pointers to the strings in the list (it is basically
only used to check for set membership), so it is ok for
string_list_clear to free them.

Signed-off-by: Martin Ågren <redacted>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cca55510..22daca2ba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2407,7 +2407,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 			       "ref_transaction_prepare");
 	size_t i;
 	int ret = 0;
-	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+	struct string_list affected_refnames = STRING_LIST_INIT_DUP;
 	char *head_ref = NULL;
 	int head_type;
 	struct object_id head_oid;

-- 
2.14.1.151.g45c1275a3.dirty

[PATCH 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

From: Martin Ågren <hidden>
Date: 2017-08-25 18:49:55

We hand over the "referent" string (buffer) to several different
functions, one of which would sometimes keep the raw pointer,
referent.buf. (When split_symref_update calls string_list_insert.) The
previous patch removed that behavior, so we can now safely release the
string buffer before returning.

Signed-off-by: Martin Ågren <redacted>
---
I appreciate Git's stance on memory leaks ("if we're about to exit or
die, why bother?") but this one feels different since
lock_ref_for_update is called in a loop rather deep down the call stack.
Feel free to tell me I'm wrong. Patch 1/2 does introduce some
malloc-churning. An alternative would be to call strbuf_release only
when we know it's safe, but that feels really ugly.

 refs/files-backend.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22daca2ba..6af07c404 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2252,7 +2252,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	struct strbuf referent = STRBUF_INIT;
 	int mustexist = (update->flags & REF_HAVE_OLD) &&
 		!is_null_oid(&update->old_oid);
-	int ret;
+	int ret = 0;
 	struct ref_lock *lock;
 
 	files_assert_main_repository(refs, "lock_ref_for_update");

@@ -2264,7 +2264,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 		ret = split_head_update(update, transaction, head_ref,
 					affected_refnames, err);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	ret = lock_raw_ref(refs, update->refname, mustexist,

@@ -2278,7 +2278,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 		strbuf_addf(err, "cannot lock ref '%s': %s",
 			    original_update_refname(update), reason);
 		free(reason);
-		return ret;
+		goto out;
 	}
 
 	update->backend_data = lock;

@@ -2297,10 +2297,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 					strbuf_addf(err, "cannot lock ref '%s': "
 						    "error reading reference",
 						    original_update_refname(update));
-					return -1;
+					ret = -1;
+					goto out;
 				}
 			} else if (check_old_oid(update, &lock->old_oid, err)) {
-				return TRANSACTION_GENERIC_ERROR;
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto out;
 			}
 		} else {
 			/*

@@ -2314,13 +2316,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 						  referent.buf, transaction,
 						  affected_refnames, err);
 			if (ret)
-				return ret;
+				goto out;
 		}
 	} else {
 		struct ref_update *parent_update;
 
-		if (check_old_oid(update, &lock->old_oid, err))
-			return TRANSACTION_GENERIC_ERROR;
+		if (check_old_oid(update, &lock->old_oid, err)) {
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto out;
+		}
 
 		/*
 		 * If this update is happening indirectly because of a

@@ -2357,7 +2361,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 				    "cannot update ref '%s': %s",
 				    update->refname, write_err);
 			free(write_err);
-			return TRANSACTION_GENERIC_ERROR;
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto out;
 		} else {
 			update->flags |= REF_NEEDS_COMMIT;
 		}

@@ -2371,10 +2376,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 		if (close_ref(lock)) {
 			strbuf_addf(err, "couldn't close '%s.lock'",
 				    update->refname);
-			return TRANSACTION_GENERIC_ERROR;
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto out;
 		}
 	}
-	return 0;
+
+out:
+	strbuf_release(&referent);
+	return ret;
 }
 
 /*

-- 
2.14.1.151.g45c1275a3.dirty

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help