The current implementation of talloc_zfree macro has a substantial deficiency: it deallocates memory first and then clears a pointer.
Clearing pointer after deallocation completes could lead to bad side effects when callbacks are called during object destruction: the callback called from destructor could dereference half-destroyed object referenced by the yet-not-cleared pointer.
The above could lead to disastrous effects including memory double free.
The patch attached fixes the issues by using correct free sequence: clear pointer first, then deallocate memory block.
Eugene
On 04/16/2010 02:02 AM, Eugene Indenbom wrote:
The current implementation of talloc_zfree macro has a substantial deficiency: it deallocates memory first and then clears a pointer.
Clearing pointer after deallocation completes could lead to bad side effects when callbacks are called during object destruction: the callback called from destructor could dereference half-destroyed object referenced by the yet-not-cleared pointer.
The above could lead to disastrous effects including memory double free.
The patch attached fixes the issues by using correct free sequence: clear pointer first, then deallocate memory block.
Eugene
My only comment with this patch is that I don't understand why you use (ptr) instead of just ptr.
Otherwise, this patch seems intuitively correct, and I like it.
On 04/16/2010 03:00 PM, Stephen Gallagher wrote:
On 04/16/2010 02:02 AM, Eugene Indenbom wrote:
The current implementation of talloc_zfree macro has a substantial deficiency: it deallocates memory first and then clears a pointer.
Clearing pointer after deallocation completes could lead to bad side effects when callbacks are called during object destruction: the callback called from destructor could dereference half-destroyed object referenced by the yet-not-cleared pointer.
The above could lead to disastrous effects including memory double free.
The patch attached fixes the issues by using correct free sequence: clear pointer first, then deallocate memory block.
Eugene
My only comment with this patch is that I don't understand why you use (ptr) instead of just ptr.
Otherwise, this patch seems intuitively correct, and I like it.
That is some kind of macro writing paranoia: always to surround macro parameters with parentheses. It should be OK without them, but...
Eugene
On 04/16/2010 07:07 AM, Eugene Indenbom wrote:
On 04/16/2010 03:00 PM, Stephen Gallagher wrote:
On 04/16/2010 02:02 AM, Eugene Indenbom wrote:
The current implementation of talloc_zfree macro has a substantial deficiency: it deallocates memory first and then clears a pointer.
Clearing pointer after deallocation completes could lead to bad side effects when callbacks are called during object destruction: the callback called from destructor could dereference half-destroyed object referenced by the yet-not-cleared pointer.
The above could lead to disastrous effects including memory double free.
The patch attached fixes the issues by using correct free sequence: clear pointer first, then deallocate memory block.
Eugene
My only comment with this patch is that I don't understand why you use (ptr) instead of just ptr.
Otherwise, this patch seems intuitively correct, and I like it.
That is some kind of macro writing paranoia: always to surround macro parameters with parentheses. It should be OK without them, but...
It would be my preference to leave the parentheses out when not needed. As someone who does both C and Python programming, I looked at this and was trying to figure out at first why you were using a tuple here :)
Since this is safe without the parens, I think it would be more readable to do without.
But as I said before: functionally I approve this patch.
On 04/16/2010 03:11 PM, Stephen Gallagher wrote:
On 04/16/2010 07:07 AM, Eugene Indenbom wrote:
On 04/16/2010 03:00 PM, Stephen Gallagher wrote:
On 04/16/2010 02:02 AM, Eugene Indenbom wrote:
The current implementation of talloc_zfree macro has a substantial deficiency: it deallocates memory first and then clears a pointer.
Clearing pointer after deallocation completes could lead to bad side effects when callbacks are called during object destruction: the callback called from destructor could dereference half-destroyed object referenced by the yet-not-cleared pointer.
The above could lead to disastrous effects including memory double free.
The patch attached fixes the issues by using correct free sequence: clear pointer first, then deallocate memory block.
Eugene
My only comment with this patch is that I don't understand why you use (ptr) instead of just ptr.
Otherwise, this patch seems intuitively correct, and I like it.
That is some kind of macro writing paranoia: always to surround macro parameters with parentheses. It should be OK without them, but...
It would be my preference to leave the parentheses out when not needed. As someone who does both C and Python programming, I looked at this and was trying to figure out at first why you were using a tuple here :)
Since this is safe without the parens, I think it would be more readable to do without.
But as I said before: functionally I approve this patch.
Patch without extra parenthesis is attached.
Eugene
On 04/16/2010 07:17 AM, Eugene Indenbom wrote:
Patch without extra parenthesis is attached.
Eugene
Ack!
On 04/16/2010 07:21 AM, Stephen Gallagher wrote:
On 04/16/2010 07:17 AM, Eugene Indenbom wrote:
Patch without extra parenthesis is attached.
Eugene
Ack!
Pushed to master and sssd-1-2.
sssd-devel@lists.fedorahosted.org
-
Eugene Indenbom -
Stephen Gallagher