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

2014-05-14 Thread Ronnie Sahlberg
Thanks.
I changed the commit message.

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?  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.

 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


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

2014-05-13 Thread Jonathan Nieder
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?  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.

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


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

2014-05-13 Thread Ronnie Sahlberg
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