Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free

On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.

In extension = as a result?

Makes sense.  It lets someone do the usual

        struct ref_transaction *transaction;
        int ret = 0;

        if (something_fails()) {
                ret = -1;
                goto cleanup;
        }
        ...

 cleanup:
        ref_transaction_free(transaction);
        return ret;

just like you can already do with free().

quoted

This allows us to write code that will

  if ( (!transaction ||
        ref_transaction_update(...))  ||
      (ref_transaction_commit(...) && !(transaction = NULL)) {
          ref_transaction_rollback(transaction);
          ...
  }

Somewhere in the whitespace and parentheses I'm lost.

Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free?

Yes. But also, this horribleness is also to illustrate a weak point in the API
in that ref_transaction_commit actually frees the transaction if
successful, so the
&& !(transaction = NULL) kludge is to avoid a double free in the
ref_transaction_rollback.

This is horrible,  but all this is going away later in the patch
series when _commit is fixed so that
it does not free the transaction anymore.
When that patch comes in later in this series, this horribleness will go away.


 I think it would be simpler for the caller to

unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.

Yes. Later patches does that by having ref_transaction_commit not free
the transaction
and instead requiring the caller to explicitely free it by calling
ref_transaction_free.


Maybe see this as this is how ugly rollback is by the current _commit
semantics. Then see how beautiful it
all gets once _commit is repaired and the  && !(transaction = NULL)
kludge is removed. :-)

Even better if it is the caller's responsibility to free
the transaction.  At any rate, it doesn't seem related to this
patch.

quoted

--- a/refs.c
+++ b/refs.c

@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
 {
      int i;

+     if (!transaction)
+             return;

Except for the unclear commit message,
Reviewed-by: Jonathan Nieder <redacted>