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-0600Date Closed:2014-11-08 18:40:24.000-0600
Versions:SVN 11.14.0 12.6.1 13.0.0 Frequency of
duplicatesASTERISK-19420 Segfault in chan_mgcp
duplicatesASTERISK-17797 Coding error in find_subchannel_and_lock()
is related toASTERISK-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.

@@ -1804,14 +1737,12 @@
       if (at && (at[0] == '[')) {
               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)) &&

Following this, parts of the code like the chunk below were removed since it's now done in the for() line:
@@ -1841,48 +1770,21 @@
                       } else {
-                               if(!g->next)
-                                       g = find_realtime_gw(name, at, sin);
-                               else
-                                       g = g->next;

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.