[1/9] refs: fix segfault when aborting empty transaction
diff mbox series

Message ID 3c7f8c47f361c46751629b44570657ef811d94d0.1585129842.git.ps@pks.im
State New
Headers show
Series
  • Support for transactions in `git-update-ref --stdin`
Related show

Commit Message

Patrick Steinhardt March 25, 2020, 9:53 a.m. UTC
When cleaning up a transaction that has no updates queued, then the
transaction's backend data will not have been allocated. We correctly
handle this for the packed backend, where the cleanup function checks
whether the backend data has been allocated at all -- if not, then there
is nothing to clean up. For the files backend we do not check this and
as a result will hit a segfault due to dereferencing a `NULL` pointer
when cleaning up such a transaction.

Fix the issue by checking whether `backend_data` is set in the files
backend, too.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Junio C Hamano March 27, 2020, 7:59 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> When cleaning up a transaction that has no updates queued, then the
> transaction's backend data will not have been allocated. We correctly
> handle this for the packed backend, where the cleanup function checks
> whether the backend data has been allocated at all -- if not, then there
> is nothing to clean up. For the files backend we do not check this and
> as a result will hit a segfault due to dereferencing a `NULL` pointer
> when cleaning up such a transaction.
>
> Fix the issue by checking whether `backend_data` is set in the files
> backend, too.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)

Makes sense.  Will queue.  Thanks.

>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 561c33ac8a..6516c7bc8c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
>  		}
>  	}
>  
> -	if (backend_data->packed_transaction &&
> -	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> -		error("error aborting transaction: %s", err.buf);
> -		strbuf_release(&err);
> +	if (backend_data) {
> +		if (backend_data->packed_transaction &&
> +		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> +			error("error aborting transaction: %s", err.buf);
> +			strbuf_release(&err);
> +		}
> +
> +		if (backend_data->packed_refs_locked)
> +			packed_refs_unlock(refs->packed_ref_store);
> +
> +		free(backend_data);
>  	}
>  
> -	if (backend_data->packed_refs_locked)
> -		packed_refs_unlock(refs->packed_ref_store);
> -
> -	free(backend_data);
> -
>  	transaction->state = REF_TRANSACTION_CLOSED;
>  }

Patch
diff mbox series

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@  static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }