On 01/16/2012 09:59 PM, Wes Hardaker wrote: >>>>>> On Thu, 12 Jan 2012 13:14:28 +0100, Jan Safranek <[email protected]> >>>>>> said: > > JS> If the netsnmp_register_mib() function returns error, sometimes it frees > JS> its reginfo argument (agent_registry.c:1136) and sometimes not > JS> (agent_registry.c:1164). The caller does not know what to do and in most > JS> cases frees reginfo again (e.g. hrSWInstalledTable.c:144 and :166). > > JS> I'd guess that netsnmp_register_mib() should not free its arguments, but > JS> I want to know your opinion first - I don't dare to touch these parts of > JS> agent without a review. > > Actually, it's fairly well documented in the code description above it > that it is freed on failure. It's been that way since 2004. > > * @param reginfo Registration handler structure. > * In a case of failure, it will be freed. > > The issue is that the line in question was changed (by me) from a hard > assert() as orbert originally programmed it to a soft netsnmp_assert in > my effort to get rid of most hard asserts as people had production > daemons that stop. > > The right fix for that function, IMHO, is this: > > diff --git a/agent/agent_registry.c b/agent/agent_registry.c > index 4333515..ab5de96 100644 > --- a/agent/agent_registry.c > +++ b/agent/agent_registry.c > @@ -1161,7 +1161,9 @@ netsnmp_register_mib(const char *moduleName, > snmp_log(LOG_WARNING,"context passed during registration does not " > "equal the reginfo contextName! ('%s' != '%s')\n", > context, reginfo->contextName); > - netsnmp_assert(!"register context == reginfo->contextName"); /* > always false */ > + netsnmp_handler_registration_free(reginfo); > + netsnmp_assert_or_return(!"register context == reginfo->contextName", > + MIB_REGISTRATION_FAILED); /* always false */ > } > > /* Create the new subtree node being registered. */ > > > That will clean the pointer and then return a FAILURE.
It's not that easy, agent_registry.c:1164 was only an example. There are other places, where the reginfo is not freed before returning error from this function. In addition, Coverity tells me that there some callers use reginfo when netsnmp_register_mib() fails (-> sigsegv), again, one example for all: hrSWInstalledTable.c:143: freed_arg: "netsnmp_register_table" frees "reg". hrSWInstalledTable.c:167: double_free: Calling "netsnmp_handler_registration_free" frees pointer "reg" which has already been freed. (actually, most of USE_AFTER_FREE Coverity reports are caused by this). Jan ------------------------------------------------------------------------------ Keep Your Developer Skills Current with LearnDevNow! The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now! http://p.sf.net/sfu/learndevnow-d2d _______________________________________________ Net-snmp-coders mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
