Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#621 fix memoy leak in PromoteToGlobal()

closed-fixed
nobody
Program (402)
5
2008-03-31
2008-02-15
Bert Wesarg
No

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).

Discussion

  • Logged In: NO

    should be updated to CVS immediately

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-01

    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

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-09

    • status: open --> closed-fixed
     
  • Bert Wesarg
    Bert Wesarg
    2008-03-09

    Logged In: YES
    user_id=122956
    Originator: YES

    committed with plenty of new comments ;-)

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-09

    • labels: --> Program
     
  • Tony Balinski
    Tony Balinski
    2008-03-26

    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;
    }

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-26

    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.

     
  • Tony Balinski
    Tony Balinski
    2008-03-27

    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.

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-27

    Logged In: YES
    user_id=122956
    Originator: YES

    > So in all I think you're being overly cautious here.
    I thought that was clear from my comments in the code. I really don't expect that any of the two fprintf's ever trigger.

    Actually, I now have the opinion, that even the LockupSymbol in the PromoteToGlobal function will never return non-NULL. So this could be removed.

     
  • Tony Balinski
    Tony Balinski
    2008-03-28

    Logged In: YES
    user_id=618141
    Originator: NO

    I've tried using the currently checked in code and it breaks my macro file loading. It also never deletes an unneeded symbol so could present a memory leak, although in practice this should never happen (see below).

    I use my NEDIT_require_macro_file() macros for just-in-time macro loading. It calls load_macro_file() for its parameter if it hasn't already seen that file name. Now the rest of the macro code after the NEDIT_require_macro_file() call contains calls to the functions defined in the file to be loaded. Since these are unknown when the macro code is compiled, their identifiers end up being promoted from local symbols to global symbols. If this happens more than once, I get the "duplicate symbol" messages. What's worse is that the symbol type is reset each time to GLOBAL_SYM, so the interpreter doesn't know what kind of symbol (function) it's dealing with! I get "<function> is not a function or subroutine" dialogs.

    As for the "memory leak", the parser creates a local symbol if it can't find one
    for the identifier found. This needs promoting to global (just to change its context) when the parser decides it is in fact a function. If a situation existed whereby a symbol with the same identifier were already present in the global list, the local symbol would be redundant and should be deleted. However, since, if the global symbol already existed we wouldn't have needed to create the local symbol in the first place, so this shouldn't happen. (So your reflection about the LookupSymbol() being redundant is true, overall.)

    All this means, by the way, that you cannot have a local symbol for a variable with the same name as a function name, unless the function is compiled after the code containing the variable! This might mean that a declaration keyword ("local" or "var" spring to mind) might be good to tie a symbol down to local name scope, making it unpromotable.

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-28

    Logged In: YES
    user_id=122956
    Originator: YES

    Ohh, I think this is stranger than i thought. I currently try this macro file, to reproduce your behaviuor:

    -----test.nm-----
    a = test()

    b = test()

    define test {
    return "hello"
    }

    c = test()

    t_print(a ":" b ":" c "\n")
    -----test.nm-----

    and with the debugging code in the attached patch, I get this:

    parse.y:559:LookupSymbol: a [notfound]
    parse.y:562:InstallSymbol: a [local]
    parse.y:559:LookupSymbol: test [notfound]
    parse.y:562:InstallSymbol: test [local]
    parse.y:320:PromoteToGlobal: test [local]
    interpret.c:806:LookupSymbol: test [notfound]
    parse.y:559:LookupSymbol: b [notfound]
    parse.y:562:InstallSymbol: b [local]
    parse.y:559:LookupSymbol: test
    parse.y:320:PromoteToGlobal: test
    interpret.c:717:InstallSymbol: string #52
    macro.c:889:LookupSymbol: test
    parse.y:559:LookupSymbol: c [notfound]
    parse.y:562:InstallSymbol: c [local]
    parse.y:559:LookupSymbol: test
    parse.y:320:PromoteToGlobal: test
    parse.y:559:LookupSymbol: t_print
    parse.y:559:LookupSymbol: a [notfound]
    parse.y:562:InstallSymbol: a [local]
    interpret.c:717:InstallSymbol: string #53
    parse.y:559:LookupSymbol: b [notfound]
    parse.y:562:InstallSymbol: b [local]
    parse.y:559:LookupSymbol: c [local]
    parse.y:243:PromoteToGlobal: t_print

    plus the 'test is not a function'-error and a 'a is not defined'-error.

    note, I don't get any of the two fprintf's from PromoteToGlobal.

    I dig further and reopen this bug.

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-28

    • status: closed-fixed --> open-fixed
     
  • Bert Wesarg
    Bert Wesarg
    2008-03-28

    Logged In: YES
    user_id=122956
    Originator: YES

    File Added: symbol-debug.patch

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-28

    debugging code

     
    Attachments
  • Bert Wesarg
    Bert Wesarg
    2008-03-29

    Logged In: YES
    user_id=122956
    Originator: YES

    Ok, my previously test case has little todo with the problem, but should be handled as a separate bug (and I think this limitation is mostly known).

    I currently use the following test case, and can't get tony's described problem:

    -----autoload.nm-----
    $loaded = $empty_array

    define requires {
    file = $1
    if (file in $loaded) {
    return
    }
    $loaded[file] = 1
    load_macro_file(file)
    t_print("loaded: " file "\n")
    }
    -----autoload.nm-----

    -----test.nm-----
    define test {
    return "hello"
    }
    -----test.nm-----

    -----test.rc-----
    nedit.macroCommands: \ test:::: {\n\ requires("test.nm")\n\ a = test()\n\ b = test()\n\ t_print(a ":" b "\\n")\n\ }
    -----test.rc-----

    I then import the test.rc and run the 'test' macro menu entry several times.

    Tony, does this test case model your usage?

     
  • Tony Balinski
    Tony Balinski
    2008-03-31

    Logged In: YES
    user_id=618141
    Originator: NO

    Yes this is right. A very cut down version of the NEDIT_require_macro_file() functionality!

    I must apologise though: I think I must have mucked up my patched version using your checked-in code. I've reworked it again and, like you, never hit one of the fprintf()s you added. Since I had removed the patched version, I cannot say what I did wrong though!

    Sorry for the lost bandwidth. I would still change your code though, not to change sym->type if sym was found using LookupSymbol(), even if this should never happen. In fact, after removing the symbol from the local list, we should perhaps just do an assert() that the same name isn't still available with LookupSymbol() before adding it to the global list. (I for one always compile NEdit with -g). I think that would be enough.

    Tony

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-31

    Logged In: YES
    user_id=122956
    Originator: YES

    > Sorry for the lost bandwidth. I would still change your code though, not
    > to change sym->type if sym was found using LookupSymbol(), even if this
    > should never happen. In fact, after removing the symbol from the local
    > list, we should perhaps just do an assert() that the same name isn't still
    > available with LookupSymbol() before adding it to the global list. (I for
    > one always compile NEdit with -g). I think that would be enough.
    assert() sounds enough (even this is the first one in the whole tree, or?), can we add one in InstallSymbol() too?

    Closed again. Reason: Bug on user side ;-)

     
  • Bert Wesarg
    Bert Wesarg
    2008-03-31

    • status: open-fixed --> closed-fixed