Lukas Oberhuber wrote:
Vadim,
Thanks. I think what I'm saw in the code is that there is no way to verify
what is going on.
In case 1, yes, easy enough. But I suspect there are other places where the
same thing is happening. It might be worth doing a quick walkthrough of the
library files, maybe looking for issues of this nature (places where more
defensive coding could be applied).
In case 2, we don't know (in part because of the multithreaded nature of the
application) which one of the two objects, the ict or the evt will be freed
first, and then which of the two objects will actually free the sip message.
For sure we know that the pointer in the other one could go stale and
there's no way to do anything about it. The only solutions I know of is to
use an automatic memory manager, or to deep copy the objects or have usage
counts on the objects so free only frees when the usage count drops to 0,
ie.
void
osip_obj_free( osip_object_t * obj)
{
if (obj->usage) // all objects start with 'usage', poor man's OO
obj->usage--;
if (obj->usage == 0)
osip_free(obj);
}
Then call it with
osip_obj_free( evt->sip );
evt->sip = NULL;
And later
osip_obj_free( ict->orig_request );
ict->orig_request = NULL;
Even better, osip_obj_free( osip_object_t ** obj ) and set to NULL directly.
Of course the following is required:
ict->orig_request = evt->sip;
osip_obj_add_usage_count(ict->orig_request); // increments 'usage' by 1
Both cases happen a number of times in the code, both case 1 and 2.
Am I making sense?
Yes, you do make sense but rewamping osip, exosip and phapi this way
is a MAJOR
undertakement....
I beleive critcial code review will be better with respect
price/performance right now
Thanks
Vadim
-Lukas
Lukas Oberhuber
-----Original Message-----
From: Vadim Lebedev [mailto:[EMAIL PROTECTED]
Sent: 14 October 2006 12:30
To: Lukas Oberhuber
Subject: Re: [Wengophone-devel] Unclear pointer struct semantics in osip
library
Lukas
Lukas Oberhuber wrote:
Hello,
Hoping to stir up a little debate:
I just spent some time in the osip/eXosip libraries and was struck that the
semantics of managing 'object' allocations is unclear. I got onto the
problem because of a crash which was caused by sitting in the debugger too
long.
Two issues popped up:
1) When 'objects' are freed, sometimes the pointer to them is not set to
NULL. This could cause issues if that pointer is later dereferenced.
For example around line 446 in wifo/lobosip2/src/osip2/ict_fsm.c:
<code>
osip_message_free (evt->sip);
// my code -->> evt->sip = NULL; <<-- shouldn't this be set to avoid the
sip being used again?
</code>
Lukas, this one seems to be a genuine bug. Nice catch. Thanks
The function eventually returns and evt is used later. Don't know if
erroneously, but the code is definitely not defensive.
2) Problem 2 relates more to who is responsible for freeing objects
Here, around line 159 in the same file, the sip message is passed to the
ict
structure.
<code>
/* Here we have ict->orig_request == NULL */
ict->orig_request = evt->sip; // <<== isn't this dangerous? Who will
free?
i = osip->cb_send_message (ict, evt->sip, ict->ict_context->destination,
ict->ict_context->port, ict->out_socket);
</code>
Neither structure is freed in this function, so the question is: who is
responsible for freeing the sip 'object', 'evt' or 'ict', and will that
object be freed in the right order? In any case, the other object will have
a stale pointer hanging around if something goes wrong.
This one is more complicated. In theory this sip object must be release
only when whole transaction goes eway.
So the code appreas to be correct
Vadim
Hope this was clear to everyone.
-Lukas
Lukas Oberhuber
_______________________________________________
Wengophone-devel mailing list
Wengophone-devel@lists.openwengo.com
http://dev.openwengo.com/mailman/listinfo/wengophone-devel
_______________________________________________
Wengophone-devel mailing list
Wengophone-devel@lists.openwengo.com
http://dev.openwengo.com/mailman/listinfo/wengophone-devel
