| Summary: | ASTERISK-14908: ao2_iterator_init() does not hold a reference to the container it is iterating | ||
| Reporter: | Kevin P. Fleming (kpfleming) | Labels: | |
| Date Opened: | 2009-09-29 11:03:20 | Date Closed: | 2009-10-05 20:42:19 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Core/General |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ||
| Description: | While researching the astobj2 code to add some new functionality, I discovered that the ao2_iterator_init() function makes a copy of the container pointer it is passed, but does not increase the reference count on the container. In situations where dynamically allocated containers are being used and pointers to them are passed to (and iterators create on them by) other functions, this could lead to the container being destroyed while there are still iterators open using it. There are no current usages of astobj2 in the tree that could experience this problem, as they do not use dynamically allocated containers with short lifetimes (all existing container usage would outlive any iterators that might be used on those containers), but the API is clearly not living up to the 'automatic reference count management' that developers would expect it to have. | ||
| Comments: | By: Digium Subversion (svnbot) 2009-10-05 20:19:47 Repository: asterisk Revision: 222152 U branches/1.4/apps/app_queue.c U branches/1.4/channels/chan_iax2.c U branches/1.4/include/asterisk/astobj2.h U branches/1.4/main/astobj2.c U branches/1.4/res/res_musiconhold.c ------------------------------------------------------------------------ r222152 | kpfleming | 2009-10-05 20:19:46 -0500 (Mon, 05 Oct 2009) | 20 lines Fix ao2_iterator API to hold references to containers being iterated. See Mantis issue for details of what prompted this change. Additional notes: This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK has become an enum instead of a macro, with a name that fits our naming policy; also, it is now necessary to call ao2_iterator_destroy() on any iterator that has been created. Currently this only releases the reference to the container being iterated, but in the future this could also release other resources used by the iterator, if the iterator implementation changes to use additional resources. (closes issue ASTERISK-14908) Reported by: kpfleming Review: https://reviewboard.asterisk.org/r/383/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=222152 By: Digium Subversion (svnbot) 2009-10-05 20:27:35 Repository: asterisk Revision: 222176 _U trunk/ U trunk/apps/app_queue.c U trunk/channels/chan_console.c U trunk/channels/chan_iax2.c U trunk/channels/chan_sip.c U trunk/funcs/func_dialgroup.c U trunk/include/asterisk/astobj2.h U trunk/main/astobj2.c U trunk/res/res_calendar.c U trunk/res/res_clialiases.c U trunk/res/res_musiconhold.c U trunk/res/res_odbc.c U trunk/res/res_phoneprov.c ------------------------------------------------------------------------ r222176 | kpfleming | 2009-10-05 20:27:35 -0500 (Mon, 05 Oct 2009) | 27 lines Recorded merge of revisions 222152 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r222152 | kpfleming | 2009-10-05 20:16:36 -0500 (Mon, 05 Oct 2009) | 20 lines Fix ao2_iterator API to hold references to containers being iterated. See Mantis issue for details of what prompted this change. Additional notes: This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK has become an enum instead of a macro, with a name that fits our naming policy; also, it is now necessary to call ao2_iterator_destroy() on any iterator that has been created. Currently this only releases the reference to the container being iterated, but in the future this could also release other resources used by the iterator, if the iterator implementation changes to use additional resources. (closes issue ASTERISK-14908) Reported by: kpfleming Review: https://reviewboard.asterisk.org/r/383/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=222176 By: Digium Subversion (svnbot) 2009-10-05 20:36:12 Repository: asterisk Revision: 222185 _U branches/1.6.0/ U branches/1.6.0/apps/app_queue.c U branches/1.6.0/channels/chan_console.c U branches/1.6.0/channels/chan_iax2.c U branches/1.6.0/funcs/func_dialgroup.c U branches/1.6.0/include/asterisk/astobj2.h U branches/1.6.0/main/astobj2.c U branches/1.6.0/res/res_musiconhold.c U branches/1.6.0/res/res_phoneprov.c ------------------------------------------------------------------------ r222185 | kpfleming | 2009-10-05 20:36:12 -0500 (Mon, 05 Oct 2009) | 34 lines Merged revisions 222176 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r222176 | kpfleming | 2009-10-05 20:24:24 -0500 (Mon, 05 Oct 2009) | 27 lines Recorded merge of revisions 222152 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r222152 | kpfleming | 2009-10-05 20:16:36 -0500 (Mon, 05 Oct 2009) | 20 lines Fix ao2_iterator API to hold references to containers being iterated. See Mantis issue for details of what prompted this change. Additional notes: This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK has become an enum instead of a macro, with a name that fits our naming policy; also, it is now necessary to call ao2_iterator_destroy() on any iterator that has been created. Currently this only releases the reference to the container being iterated, but in the future this could also release other resources used by the iterator, if the iterator implementation changes to use additional resources. (closes issue ASTERISK-14908) Reported by: kpfleming Review: https://reviewboard.asterisk.org/r/383/ ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=222185 By: Digium Subversion (svnbot) 2009-10-05 20:39:49 Repository: asterisk Revision: 222186 _U branches/1.6.1/ U branches/1.6.1/apps/app_queue.c U branches/1.6.1/channels/chan_console.c U branches/1.6.1/channels/chan_iax2.c U branches/1.6.1/channels/chan_sip.c U branches/1.6.1/funcs/func_dialgroup.c U branches/1.6.1/include/asterisk/astobj2.h U branches/1.6.1/main/astobj2.c U branches/1.6.1/res/res_musiconhold.c U branches/1.6.1/res/res_odbc.c U branches/1.6.1/res/res_phoneprov.c ------------------------------------------------------------------------ r222186 | kpfleming | 2009-10-05 20:39:48 -0500 (Mon, 05 Oct 2009) | 34 lines Merged revisions 222176 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r222176 | kpfleming | 2009-10-05 20:24:24 -0500 (Mon, 05 Oct 2009) | 27 lines Recorded merge of revisions 222152 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r222152 | kpfleming | 2009-10-05 20:16:36 -0500 (Mon, 05 Oct 2009) | 20 lines Fix ao2_iterator API to hold references to containers being iterated. See Mantis issue for details of what prompted this change. Additional notes: This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK has become an enum instead of a macro, with a name that fits our naming policy; also, it is now necessary to call ao2_iterator_destroy() on any iterator that has been created. Currently this only releases the reference to the container being iterated, but in the future this could also release other resources used by the iterator, if the iterator implementation changes to use additional resources. (closes issue ASTERISK-14908) Reported by: kpfleming Review: https://reviewboard.asterisk.org/r/383/ ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=222186 By: Digium Subversion (svnbot) 2009-10-05 20:42:19 Repository: asterisk Revision: 222187 _U branches/1.6.2/ U branches/1.6.2/apps/app_queue.c U branches/1.6.2/channels/chan_console.c U branches/1.6.2/channels/chan_iax2.c U branches/1.6.2/channels/chan_sip.c U branches/1.6.2/funcs/func_dialgroup.c U branches/1.6.2/include/asterisk/astobj2.h U branches/1.6.2/main/astobj2.c U branches/1.6.2/res/res_clialiases.c U branches/1.6.2/res/res_musiconhold.c U branches/1.6.2/res/res_odbc.c U branches/1.6.2/res/res_phoneprov.c ------------------------------------------------------------------------ r222187 | kpfleming | 2009-10-05 20:42:18 -0500 (Mon, 05 Oct 2009) | 34 lines Merged revisions 222176 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r222176 | kpfleming | 2009-10-05 20:24:24 -0500 (Mon, 05 Oct 2009) | 27 lines Recorded merge of revisions 222152 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r222152 | kpfleming | 2009-10-05 20:16:36 -0500 (Mon, 05 Oct 2009) | 20 lines Fix ao2_iterator API to hold references to containers being iterated. See Mantis issue for details of what prompted this change. Additional notes: This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK has become an enum instead of a macro, with a name that fits our naming policy; also, it is now necessary to call ao2_iterator_destroy() on any iterator that has been created. Currently this only releases the reference to the container being iterated, but in the future this could also release other resources used by the iterator, if the iterator implementation changes to use additional resources. (closes issue ASTERISK-14908) Reported by: kpfleming Review: https://reviewboard.asterisk.org/r/383/ ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=222187 | ||