| Summary: | ASTERISK-16197: [patch] "setvar" can add multiple variables with the same name to a channel | ||||
| Reporter: | Nahuel Greco (nahuelgreco) | Labels: | |||
| Date Opened: | 2010-06-02 17:28:50 | Date Closed: | 2011-04-12 15:48:18 | ||
| Priority: | Minor | Regression? | No | ||
| Status: | Closed/Complete | Components: | Channels/chan_dahdi | ||
| Versions: | Frequency of Occurrence | ||||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) 20100603__issue17450__1.6.2.diff.txt ( 1) chan_dahdi.c.setvar.patch ( 2) patch.diff | |||
| Description: | Suppose the following piece of chan_dahdi.conf: setvar=V1=foo channel => 1 setvar=V2=bar channel => 2 setvar=V1=baz channel => 3 In the current trunk version, each channel (dahdi_pvt structure) will have his own copies of the variables (after ASTERISK-15249 fix). For example, the ast_variables linked list for channel 3 will have the following form: channel 3: V1=foo -> V2=bar -> V1=baz When a call is made and dahdi_new(...) is executed, dahdi_new(...) will processes the variables list left-to-right, so for a call in channel 3 the value of V1 for that call will be finally "baz" because is the last one to be set by dahdi_new(...). This makes the bug benevolous, the call variables will be ok, but if you do a "dahdi show channel 3" you will see the two versions of V1, and that's wrong. Before the ASTERISK-15249 fix the bug was worse, because the call in channel 3 got V1=foo. A patch against trunk removing the duplicates is attached. Also, there is a conceptual mismatch. The documentation in chan_dahdi.conf.sample states that "setvar" defines a "Channel variable to be set for all calls from this channel". Maybe the correct behaviour is to get only V1 defined for channel 3 and not V2, by resetting the setvar list (confp->chan.vars) after each "channel" statement. I also think the linked list built at confp->chan.vars is probably leaking across reloads. | ||||
| Comments: | By: Tilghman Lesher (tilghman) 2010-06-03 01:33:56 1) I agree on the leak, and I'm uploading a fix for that. 2) I don't agree on skipping variables and here's why: we have dialplan functions which may be set with this method, and those don't have a particular problem with being set more than once. In fact, each setting may depend upon the previous value internally, which makes skipping variables problematic. 3) Inheritance of previous values is a feature, not a bug. If you'd like an explicit "clearvars" command, as we have in chan_mgcp, I'm open to that. By: Russell Bryant (russell) 2010-06-03 10:01:37 Tilghman, perhaps we should just detect when it's a function and allow it in that case? Having multiple normal variables of the same name certainly seems wrong ... By: Nahuel Greco (nahuelgreco) 2010-06-03 13:03:42 tlighman: can you give me a short example of your second point ? By: Tilghman Lesher (tilghman) 2010-06-03 13:37:42 Sure, for example, a func_odbc function might directly use a channel variable, and you'd need to set it initially for the setup and again, after using the func_odbc function. setvar=chanstate=setup setvar=ODBC_FOO()=1 setvar=chanstate=dialling By: Jonathan Rose (jrose) 2011-04-08 15:30:00 reload chan_dahdi doesn't leak any memory and according to a few of our devs, the way variables get stored in the pvt struct like that is fine since once it reaches the status of being a channel there is really only one variable for each name anyway. I'm closing this issue. If there is some specific memory leak that you can demonstrate with malloc_debug (memory show summary), please create a new issue so that we can investigate it more thoroughly. By: Tilghman Lesher (tilghman) 2011-04-10 14:13:35 jrose: there is no leak unless you have your configuration file a certain way. My patch fixes this, and it still applies. EDIT: if you're wondering how to reproduce, create a few setvar= values in your users.conf in [general], or a few setvar= values in chan_dahdi.conf in [channels], and reload dahdi a couple times (make sure you 'touch users.conf chan_dahdi.conf' between reloads). There's your memory leak. By: Jonathan Rose (jrose) 2011-04-12 12:46:41 I mentioned in IRC that I'd noted the leak. I've made a patch that I think fixes it since yours didn't work for me. By: Digium Subversion (svnbot) 2011-04-12 13:12:30 Repository: asterisk Revision: 313432 U branches/1.6.2/channels/chan_dahdi.c ------------------------------------------------------------------------ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines fixes a chan_dahdi reload memory leak that occurs when there are channel variables reload chan_dahdi would cause variables to spill over from the previous config into the current config as well as add any variables in the new config (if left the same, full on duplication) This patch purges variables from pvts when they are reconfigured as well as from the configuration pvt once the configuration has been loaded. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313432 By: Digium Subversion (svnbot) 2011-04-12 13:25:49 Repository: asterisk Revision: 313434 _U branches/1.8/ U branches/1.8/channels/chan_dahdi.c ------------------------------------------------------------------------ r313434 | jrose | 2011-04-12 13:25:49 -0500 (Tue, 12 Apr 2011) | 21 lines Merged revisions 313432 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines fixes a chan_dahdi reload memory leak that occurs when there are channel variables reload chan_dahdi would cause variables to spill over from the previous config into the current config as well as add any variables in the new config (if left the same, full on duplication) This patch purges variables from pvts when they are reconfigured as well as from the configuration pvt once the configuration has been loaded. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313434 By: Digium Subversion (svnbot) 2011-04-12 15:25:06 Repository: asterisk Revision: 313432 U branches/1.6.2/channels/chan_dahdi.c ------------------------------------------------------------------------ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 16 lines fixes reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313432 By: Digium Subversion (svnbot) 2011-04-12 15:27:07 Repository: asterisk Revision: 313433 U branches/1.6.2/channels/chan_dahdi.c ------------------------------------------------------------------------ r313433 | jrose | 2011-04-12 13:19:41 -0500 (Tue, 12 Apr 2011) | 22 lines white space change ........ reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313433 By: Digium Subversion (svnbot) 2011-04-12 15:28:13 Repository: asterisk Revision: 313434 _U branches/1.8/ U branches/1.8/channels/chan_dahdi.c ------------------------------------------------------------------------ r313434 | jrose | 2011-04-12 13:25:48 -0500 (Tue, 12 Apr 2011) | 25 lines Merged revisions 313432 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313434 By: Digium Subversion (svnbot) 2011-04-12 15:29:04 Repository: asterisk Revision: 313433 U branches/1.6.2/channels/chan_dahdi.c ------------------------------------------------------------------------ r313433 | jrose | 2011-04-12 13:19:41 -0500 (Tue, 12 Apr 2011) | 23 lines white space change ........ reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313433 By: Digium Subversion (svnbot) 2011-04-12 15:35:23 Repository: asterisk Revision: 313435 U branches/1.6.2/channels/chan_dahdi.c ------------------------------------------------------------------------ r313435 | jrose | 2011-04-12 13:44:44 -0500 (Tue, 12 Apr 2011) | 30 lines fixing stupid mistake with putting code before variable declaration ........ Merged revisions 313433 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313435 By: Digium Subversion (svnbot) 2011-04-12 15:40:28 Repository: asterisk Revision: 313436 _U branches/1.8/ U branches/1.8/channels/chan_dahdi.c ------------------------------------------------------------------------ r313436 | jrose | 2011-04-12 13:47:05 -0500 (Tue, 12 Apr 2011) | 43 lines fixing stupid mistake with putting code before variable declaration ........ Merged revisions 313435 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313436 By: Digium Subversion (svnbot) 2011-04-12 15:48:17 Repository: asterisk Revision: 313437 _U trunk/ U trunk/channels/chan_dahdi.c ------------------------------------------------------------------------ r313437 | jrose | 2011-04-12 13:50:11 -0500 (Tue, 12 Apr 2011) | 39 lines Merged revisions 313435 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 also went ahead and fixed the problem it introduces before committing. ........ r313435 | jrose | 2011-04-12 13:44:44 -0500 (Tue, 12 Apr 2011) | 1 line fixing stupid mistake with putting code before variable declaration ........ Merged revisions 313433 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r313432 | jrose | 2011-04-12 13:12:29 -0500 (Tue, 12 Apr 2011) | 14 lines reload Chan_dahdi memory leak caused by variables chan_dahdi reloading with variables set via setvar in chan_dahdi.conf would stay in the dahdi_pvt structs for individual channels (causing them to just continue adding the new ones to the list) and also there was a memory leak causes by the conf objects. This patch resolves both of these by using ast_variables_destroy during the loading process. (closes issue ASTERISK-16197) Reported by: nahuelgreco Patches: patch.diff uploaded by jrose (license 1225) Tested by: tilghman, jrose Review: https://reviewboard.asterisk.org/r/1170/ ........ ........ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=313437 | ||||