Summary:	ASTERISK-24500: Regression introduced in chan_mgcp by SVN revision r227276
Reporter:	Xavier Hienne (xhienne)	Labels:
Date Opened:	2014-11-05 10:46:08.000-0600	Date Closed:	2014-11-08 18:40:24.000-0600
Priority:	Major	Regression?	Yes
Status:	Closed/Complete	Components:	Channels/chan_mgcp
Versions:	SVN 1.8.32.0 11.14.0 12.6.1 13.0.0	Frequency of Occurrence
Related Issues:	duplicates ASTERISK-19420 Segfault in chan_mgcp duplicates ASTERISK-17797 Coding error in find_subchannel_and_lock() is related to ASTERISK-12278 [patch] PacketCable NCS 1.0 Support for Docsis / Eurodocsis Networks
Environment:		Attachments:	( 0) chan_mgcp.patch
Description:	In SVN revision r227276, a while() loop was turned into a for() loop. {code} @@ -1804,14 +1737,12 @@        if (at && (at[0] == '[')) {                at++;                c = strrchr(at, ']'); -               if (c) +               if (c) {                        *c = '\0'; +               }        } -       g = gateways; -       if(!g) -               g = find_realtime_gw(name, at, sin); -       while(g) { -               if ((!name || !strcasecmp(g->name, at)) && +       for (g = gateways ? gateways : find_realtime_gw(name, at, sin); g; g = g->next ? g->next : find_realtime_gw(name, at, sin)) { +               if ((!name || !strcasecmp(g->name, at)) && {code} Following this, parts of the code like the chunk below were removed since it's now done in the for() line: {code} @@ -1841,48 +1770,21 @@                                        continue;                                }                        } else { -                               if(!g->next) -                                       g = find_realtime_gw(name, at, sin); -                               else -                                       g = g->next;                                continue;                        } {code} The problem is that one such code chunk was forgotten and is still here in the "else if (!name && sin)" block, duplicating what is already done in the for() line. The end result is that instead of going through the linked list of MGCP gateways one by one, when a static unnamed MGCP GW is encountered in the list, the next GW is skipped.
Comments:	By: Rusty Newton (rnewton) 2014-11-05 16:55:33.067-0600 Xavier would you like to provide a patch and testing notes?  Since chan_mgcp is extended support (supported by the broader community) and not used by many it'll have a greater chance of getting fixed if you can assist. See the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process] if you are interested! By: Xavier Hienne (xhienne) 2014-11-06 11:41:10.194-0600 I'll gladly provide a patch but I haven't signed the Digium License Agreement (yet). I have to discuss this issue with my employer first. Can't we bypass the DLA requirement by considering this patch as trivial, since it is mostly a continuation (I mean copy-paste) of what was done in diff r227276? By: Matt Jordan (mjordan) 2014-11-06 13:36:25.918-0600 Sorry, but no. Even one liner patches have to be covered by a CLA. By: Xavier Hienne (xhienne) 2014-11-07 10:16:48.815-0600 Patch that fixes the reported bug By: Xavier Hienne (xhienne) 2014-11-07 10:29:35.308-0600 OK, DLA now signed. I have attached a patch against the Asterisk-11 branch. It should apply nicely to the 1.8, 12 and 13 branches (with some line offset though). Tested with Asterisk 11. This patch removes the remaining "g = g->next" inside the for() loop, as they are redundant with what is done in the for() statement. In the best case scenario, this bug would skip the next gateway in the list; in the worst case, it would trigger a segfault as g migh be NULL when "g->next" is evaluated in the for() statement. By: Matt Jordan (mjordan) 2014-11-07 13:36:21.058-0600 Thanks for the patch! Taking a look at the patch and the code, your analysis looks correct to me.