2015-11-06 0:48 GMT+03:00 Bram Moolenaar <[email protected]>: > > Nikolay Pavlov wrote: > >> >> Consider the following scenario: while processing function dict_copy >> >> >> >> static dict_T * >> >> dict_copy(orig, deep, copyID) >> >> dict_T *orig; >> >> int deep; >> >> int copyID; >> >> { >> >> dict_T *copy; >> >> dictitem_T *di; >> >> int todo; >> >> hashitem_T *hi; >> >> >> >> if (orig == NULL) >> >> return NULL; >> >> >> >> copy = dict_alloc(); >> >> >> >> this allocation occupied last memory in the system, >> >> >> >> if (copy != NULL) >> >> { >> >> if (copyID != 0) >> >> { >> >> orig->dv_copyID = copyID; >> >> orig->dv_copydict = copy; >> >> } >> >> todo = (int)orig->dv_hashtab.ht_used; >> >> for (hi = orig->dv_hashtab.ht_array; todo > 0 && !got_int; >> >> ++hi) >> >> { >> >> if (!HASHITEM_EMPTY(hi)) >> >> { >> >> --todo; >> >> >> >> di = dictitem_alloc(hi->hi_key); >> >> >> >> so that this allocation at first fails and results in >> >> `garbage_collect()` being called from `lalloc()` in `misc2.c`. >> >> >> >> Since `copy` dictionary is not yet referenced anywhere >> >> `garbage_collect()` call will free the dictionary and >> >> >> >> if (di == NULL) >> >> break; >> >> if (deep) >> >> { >> >> if (item_copy(&HI2DI(hi)->di_tv, &di->di_tv, deep, >> >> >> >> copyID) == FAIL) >> >> { >> >> vim_free(di); >> >> break; >> >> } >> >> } >> >> else >> >> copy_tv(&HI2DI(hi)->di_tv, &di->di_tv); >> >> if (dict_add(copy, di) == FAIL) >> >> >> >> Vim will crash at this point. >> >> >> >> { >> >> dictitem_free(di); >> >> break; >> >> } >> >> } >> >> } >> >> >> >> ++copy->dv_refcount; >> >> if (todo > 0) >> >> { >> >> dict_unref(copy); >> >> copy = NULL; >> >> } >> >> } >> >> >> >> return copy; >> >> } >> >> >> >> Please correct me if I am wrong. >> > >> > Looks like incrementing dv_refcount should happen much earlier. >> > >> > Real garbage collection is so much easier than reference counting... >> >> I do not think this will work: `garbage_collect` is protecting against >> memory leaks in cycles, so it will collect `copy` no matter what its >> reference count is, it is enough that `copyID` will not be set. > > You are right, I forgot about that. > >> Also note that this is only one of the examples: after identifying the >> problem I just found a random function that has memory allocation >> after `dict_alloc`, but before allocated dictionary is saved anywhere >> where `garbage_collect` may assign it a copyID. > > Not sure what is the best way to avoid this. Adding each new dict to a > list of "items being used" will quickly get very complicated. Probably > the best solution is to set a flag to avoid garbage_collect to be > called, just return NULL when allocating memory. Would need to do this > a lot when evaluating expressions.
I would suggest to remove `garbage_collect` call from there *at all*: 1. This is much simpler then having *yet another* layer on top of usual C memory managing problem. 2. Most of time there are no unreferenced cycles, so this function is not going to free anything. > > -- > From "know your smileys": > :-O>-o Smiley American tourist (note big mouth and camera) > > /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\ > /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ > \\\ an exciting new programming language -- http://www.Zimbu.org /// > \\\ help me help AIDS victims -- http://ICCF-Holland.org /// -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
