Bugs item #1894726, was opened at 2008-02-16 00:44 Message generated for change (Comment added) made by ajbj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=111005&aid=1894726&group_id=11005
Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Program Group: None Status: Closed Resolution: Fixed Priority: 5 Private: No Submitted By: Bert Wesarg (lebert) Assigned to: Nobody/Anonymous (nobody) Summary: fix memoy leak in PromoteToGlobal() Initial Comment: InstallSymbol() allocates a new Symbol and put this to the GlobalSymList the Symbol passed to PromoteToGlobal() is lost. This patch adds the Symbol directly to the GlobalSymList, without calling InstallSymbol(). The case in which the Symbol is already in the LocalSymbolList is not changed, because I don't know if this can a cause SIGSEGV but it should also be corrected (ie freeing sym). ---------------------------------------------------------------------- Comment By: Tony Balinski (ajbj) Date: 2008-03-27 07:42 Message: Logged In: YES user_id=618141 Originator: NO It does what the old function did (which must have been there ever since the macro language was invented!), but without the leak you discovered. As for safety, I don't think this is much of a concern. The equivalence of the LOCAL_SYM flag and the local symbol list (which gets attached to the compiled function) is enshrined in the InstallSymbol() function, which is always used one way or another to register a symbol to either the global or local context (with the SINGLE exception of my proposed change). As for assumptions, you test whether a symbol marked LOCAL_SYM is in the global list: this should really be done as an assertion since it should never happen and represents a mistake in code. The other causes complaints when a new function name is found before it is defined. This is a legitimate occurrence and causes complaints to occur with mutually recursive functions and "include file"-style function loading. So in all I think you're being overly cautious here. ---------------------------------------------------------------------- Comment By: Bert Wesarg (lebert) Date: 2008-03-26 10:21 Message: Logged In: YES user_id=122956 Originator: YES This is the optimistic version. You assume that PromoteToGlobal() has the only reference to the symbol passed. This is currently correct. If you assume this, than you can simply free the symbol and return the symbol from the GlobalSymList, in case the symbol was found there, which should not happen currently. The next assumption you made is, that a LOCAL_SYM can never be in the GlobalSymList. This is currently true, hopefully. I have tried to address both assumptions in the commit and put in fprintf's. Also, as I stated in the comment, I think these are parser errors, which should error out the parsing process. ---------------------------------------------------------------------- Comment By: Tony Balinski (ajbj) Date: 2008-03-26 01:55 Message: Logged In: YES user_id=618141 Originator: NO After consideration I don't agree with the committed solution. All that needs to be done is: if the symbol being promoted is on the local list, take it off that one, change its type to GLOBAL_SYM, then add it to the global list if it isn't already there, or delete it if it is. See http://www.nedit.org/pipermail/develop/2008-March/014355.html and http://www.nedit.org/pipermail/develop/2008-March/014365.html (my analysis was correct and Bert's response true). The solution I propose is: /* ** Promote a symbol from local to global, removing it from the local symbol ** list. ** ** The parser assumes that all non-global symbols are for local variables ** (unless already known) until it sees a following parenthesis (supposedly ** starting an argument list). When this happens, PromoteToGlobal() is called ** so that the symbol is placed on the global symbol list, since this is where ** all function symbols should be held. Since, when this occurs, there is as ** yet no function defined, we say the symbol is still that of a variable (now ** global). This will be overwritten correctly when a definition of a function ** with that symbol is read. */ Symbol *PromoteToGlobal(Symbol *sym) { Symbol *s, *global_sym; static DataValue noValue = {NO_TAG, {0}}; if (sym->type != LOCAL_SYM) return sym; /* Remove sym from the local symbol list */ if (sym == LocalSymList) LocalSymList = sym->next; else { for (s = LocalSymList; s != NULL; s = s->next) { if (s->next == sym) { s->next = sym->next; break; } } } sym->next = NULL; global_sym = LookupSymbol(sym->name); if (global_sym != NULL) { /* get rid of sym's instance - the name is already there */ freeSymbolTable(sym); } else { /* put sym on the global list */ sym->next = GlobalSymList; sym->type = GLOBAL_SYM; GlobalSymList = global_sym = sym; } return global_sym; } ---------------------------------------------------------------------- Comment By: Bert Wesarg (lebert) Date: 2008-03-09 17:40 Message: Logged In: YES user_id=122956 Originator: YES committed with plenty of new comments ;-) ---------------------------------------------------------------------- Comment By: Bert Wesarg (lebert) Date: 2008-03-01 02:07 Message: Logged In: YES user_id=122956 Originator: YES I will commit this fix by the next week. There is only the problem with the extra LookupSymbol() call. I suspect that this would be a greater error, if this call ever succeeded, because that would mean that a LOCAL_SYM was not in the LocalSymList, or there was previously a non LOCAL_SYM in the GlobalSymList. I would like to add a fprintf instead of the return, and see if this case is ever triggered (see the new patch). File Added: fix-mem-leak-in-PromoteToGlobal.patch ---------------------------------------------------------------------- Comment By: Nobody/Anonymous (nobody) Date: 2008-02-16 23:10 Message: Logged In: NO should be updated to CVS immediately ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=111005&aid=1894726&group_id=11005 -- NEdit Develop mailing list - [email protected] http://www.nedit.org/mailman/listinfo/develop
