Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free
Thanks.
I changed the commit message.
On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder 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
--
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
On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder 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
--
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
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
--
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
[PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
In this case transaction is reset to NULL IFF ref_transaction_commit() was
invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
is safe. IF the conditional triggered prior to ref_transaction_commit()
then transaction is untouched and then ref_transaction_rollback(transaction)
will rollback the failed transaction.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 138ab70..2d83ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i < transaction->nr; i++)
free(transaction->updates[i]);
--
2.0.0.rc1.351.g4d2c8e4
--
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
