[asterisk-dev] possible memory leak in chan_sip.c ?
Luigi Rizzo
rizzo at icir.org
Thu Nov 9 09:27:58 MST 2006
any ideas on the second part, __sip_ack() leaking memory ?
cheers
luigi
On Thu, Nov 09, 2006 at 04:19:05PM -0000, asterisk-commits at lists.digium.com wrote:
> Author: rizzo
> Date: Thu Nov 9 10:19:05 2006
> New Revision: 47373
>
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=47373
> Log:
> rename the "owner" field in a sip_pkt to "pvt", as it
> is too confusing otherwise ("owner" is used to indicate a
> channel in a nearby struct). Most of the usages are in a
> single function.
>
> Trunk candidate, definitely.
>
> On passing note that __sip_ack(), when called by handle_response()
> with resp == 491, does not free the packet, but because
> the packet is not linked in the list anymore, this possibly
> causes a memory leak. If not, it is a very obscure thing.
>
> Need to consider this as a possible bug in trunk.
>
>
> Modified:
> team/rizzo/astobj2/channels/chan_sip.c
>
> Modified: team/rizzo/astobj2/channels/chan_sip.c
> URL: http://svn.digium.com/view/asterisk/team/rizzo/astobj2/channels/chan_sip.c?view=diff&rev=47373&r1=47372&r2=47373
> ==============================================================================
> --- team/rizzo/astobj2/channels/chan_sip.c (original)
> +++ team/rizzo/astobj2/channels/chan_sip.c Thu Nov 9 10:19:05 2006
> @@ -1056,7 +1056,7 @@
> int method; /*!< SIP method for this packet */
> int seqno; /*!< Sequence number */
> unsigned int flags; /*!< non-zero if this is a response packet (e.g. 200 OK) */
> - struct sip_pvt *owner; /*!< Owner AST call */
> + struct sip_pvt *pvt; /*!< Owner AST call */
> int retransid; /*!< Retransmission ID */
> int timer_a; /*!< SIP timer A, retransmission timer */
> int timer_t1; /*!< SIP Timer T1, estimated RTT or 500 ms */
> @@ -1945,9 +1945,10 @@
> {
> struct sip_pkt *pkt = data, *prev, *cur = NULL;
> int reschedule = DEFAULT_RETRANS;
> + struct sip_pvt *pvt = pkt->pvt; /* XXX we assume it is not null. maybe should check ? */
>
> /* Lock channel PVT */
> - sip_pvt_lock(pkt->owner);
> + sip_pvt_lock(pvt);
>
> if (pkt->retrans < MAX_RETRANS) {
> pkt->retrans++;
> @@ -1975,48 +1976,49 @@
> ast_log(LOG_DEBUG, "** SIP timers: Rescheduling retransmission %d to %d ms (t1 %d ms (Retrans id #%d)) \n", pkt->retrans +1, siptimer_a, pkt->timer_t1, pkt->retransid);
> }
>
> - if (sip_debug_test_pvt(pkt->owner)) {
> - const struct sockaddr_in *dst = sip_real_dst(pkt->owner);
> + if (sip_debug_test_pvt(pkt->pvt)) {
> + const struct sockaddr_in *dst = sip_real_dst(pvt);
> ast_verbose("Retransmitting #%d (%s) to %s:%d:\n%s\n---\n",
> - pkt->retrans, sip_nat_mode(pkt->owner),
> + pkt->retrans, sip_nat_mode(pvt),
> ast_inet_ntoa(dst->sin_addr),
> ntohs(dst->sin_port), pkt->data);
> }
>
> - append_history(pkt->owner, "ReTx", "%d %s", reschedule, pkt->data);
> - __sip_xmit(pkt->owner, pkt->data, pkt->packetlen);
> - sip_pvt_unlock(pkt->owner);
> + append_history(pvt, "ReTx", "%d %s", reschedule, pkt->data);
> + __sip_xmit(pvt, pkt->data, pkt->packetlen);
> + sip_pvt_unlock(pvt);
> return reschedule;
> }
> /* Too many retries */
> - if (pkt->owner && pkt->method != SIP_OPTIONS) {
> + if (pkt->method != SIP_OPTIONS) {
> if (ast_test_flag(pkt, FLAG_FATAL) || sipdebug) /* Tell us if it's critical or if we're debugging */
> - ast_log(LOG_WARNING, "Maximum retries exceeded on transmission %s for seqno %d (%s %s)\n", pkt->owner->callid, pkt->seqno, (ast_test_flag(pkt, FLAG_FATAL)) ? "Critical" : "Non-critical", (ast_test_flag(pkt, FLAG_RESPONSE)) ? "Response" : "Request");
> + ast_log(LOG_WARNING, "Maximum retries exceeded on transmission %s for seqno %d (%s %s)\n", pvt->callid, pkt->seqno, (ast_test_flag(pkt, FLAG_FATAL)) ? "Critical" : "Non-critical", (ast_test_flag(pkt, FLAG_RESPONSE)) ? "Response" : "Request");
> } else {
> if ((pkt->method == SIP_OPTIONS) && sipdebug)
> - ast_log(LOG_WARNING, "Cancelling retransmit of OPTIONs (call id %s) \n", pkt->owner->callid);
> - }
> - append_history(pkt->owner, "MaxRetries", "%s", (ast_test_flag(pkt, FLAG_FATAL)) ? "(Critical)" : "(Non-critical)");
> + ast_log(LOG_WARNING, "Cancelling retransmit of OPTIONs (call id %s) \n", pvt->callid);
> + }
> + append_history(pvt, "MaxRetries", "%s", (ast_test_flag(pkt, FLAG_FATAL)) ? "(Critical)" : "(Non-critical)");
>
> pkt->retransid = -1;
>
> if (ast_test_flag(pkt, FLAG_FATAL)) {
> /* the next call is unsafe, because we are called without dialoglock held,
> - * and pkt->owner could disappear while we unlock it
> + * and pvt could disappear while we unlock it
> */
> - lock_pvt_and_owner(pkt->owner, 0 /* try forever */);
> - if (pkt->owner->owner) {
> - ast_set_flag(&pkt->owner->flags[0], SIP_ALREADYGONE);
> - ast_log(LOG_WARNING, "Hanging up call %s - no reply to our critical packet.\n", pkt->owner->callid);
> - ast_queue_hangup(pkt->owner->owner);
> - ast_channel_unlock(pkt->owner->owner);
> + lock_pvt_and_owner(pvt, 0 /* try forever */);
> + if (pvt->owner) {
> + ast_set_flag(&pvt->flags[0], SIP_ALREADYGONE);
> + ast_log(LOG_WARNING, "Hanging up call %s - no reply to our critical packet.\n", pvt->callid);
> + ast_queue_hangup(pvt->owner);
> + ast_channel_unlock(pvt->owner);
> } else {
> /* If no channel owner, destroy now */
> - ast_set_flag(&pkt->owner->flags[0], SIP_NEEDDESTROY);
> - }
> - }
> + ast_set_flag(&pvt->flags[0], SIP_NEEDDESTROY);
> + }
> + }
> + /* XXX this code needs to be fixed */
> /* In any case, go ahead and remove the packet */
> - for (prev = NULL, cur = pkt->owner->packets; cur; prev = cur, cur = cur->next) {
> + for (prev = NULL, cur = pvt->packets; cur; prev = cur, cur = cur->next) {
> if (cur == pkt)
> break;
> }
> @@ -2024,14 +2026,15 @@
> if (prev)
> prev->next = cur->next;
> else
> - pkt->owner->packets = cur->next;
> - sip_pvt_unlock(pkt->owner);
> + pvt->packets = cur->next;
> + pkt->pvt = pvt_unref(pvt); /* release the pvt */
> + sip_pvt_unlock(pvt);
> free(cur);
> pkt = NULL;
> } else
> ast_log(LOG_WARNING, "Weird, couldn't find packet owner!\n");
> if (pkt)
> - sip_pvt_unlock(pkt->owner);
> + sip_pvt_unlock(pvt);
> return 0;
> }
>
> @@ -2049,7 +2052,7 @@
> pkt->method = sipmethod;
> pkt->packetlen = len;
> pkt->next = p->packets;
> - pkt->owner = p;
> + pkt->pvt = pvt_ref(p);
> pkt->seqno = seqno;
> if (resp)
> ast_set_flag(pkt, FLAG_RESPONSE);
> @@ -2067,7 +2070,7 @@
> pkt->next = p->packets;
> p->packets = pkt;
>
> - __sip_xmit(pkt->owner, pkt->data, pkt->packetlen); /* Send packet */
> + __sip_xmit(pkt->pvt, pkt->data, pkt->packetlen); /* Send packet */
> if (sipmethod == SIP_INVITE) {
> /* Note this is a pending invite */
> p->pendinginvite = seqno;
> @@ -2167,6 +2170,8 @@
> ast_log(LOG_DEBUG, "** SIP TIMER: Cancelling retransmit of packet (reply received) Retransid #%d\n", cur->retransid);
> ast_sched_del(sched, cur->retransid);
> }
> + if (cur->pvt) /* XXX should be always */
> + cur->pvt = pvt_unref(cur->pvt);
> if (!reset)
> free(cur);
> break;
> @@ -12388,7 +12393,7 @@
> if ((resp >= 100) && (resp <= 199))
> __sip_semi_ack(p, seqno, 0, sipmethod);
> else
> - __sip_ack(p, seqno, 0, sipmethod, resp == 491 ? TRUE : FALSE);
> + __sip_ack(p, seqno, 0, sipmethod, resp == 491 ? TRUE : FALSE); /* XXX why true on 491 ? */
>
> /* Get their tag if we haven't already */
> if (ast_strlen_zero(p->theirtag) || (resp >= 200)) {
>
> _______________________________________________
> --Bandwidth and Colocation provided by Easynews.com --
>
> asterisk-commits mailing list
> To UNSUBSCRIBE or update options visit:
> http://lists.digium.com/mailman/listinfo/asterisk-commits
More information about the asterisk-dev
mailing list