On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder <jrnie...@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> 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().
>
>> 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.
>
>> --- 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 <jrnie...@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to