any ideas on the second part, __sip_ack() leaking memory ? cheers luigi
On Thu, Nov 09, 2006 at 04:19:05PM -0000, [EMAIL PROTECTED] 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 _______________________________________________ --Bandwidth and Colocation provided by Easynews.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
