{"action":"created","issue":{"url":"https://api.github.com/repos/neovim/neovim/issues/3630","labels_url":"https://api.github.com/repos/neovim/neovim/issues/3630/labels{/name}","comments_url":"https://api.github.com/repos/neovim/neovim/issues/3630/comments","events_url":"https://api.github.com/repos/neovim/neovim/issues/3630/events","html_url":"https://github.com/neovim/neovim/pull/3630","id":115726293,"number":3630,"title":"[RFC] eval.c: Protect saved funccalls against garbage collection","user":{"login":"oni-link","id":2077655,"avatar_url":"https://avatars.githubusercontent.com/u/2077655?v=3","gravatar_id":"","url":"https://api.github.com/users/oni-link","html_url":"https://github.com/oni-link","followers_url":"https://api.github.com/users/oni-link/followers","following_url":"https://api.github.com/users/oni-link/following{/other_user}","gists_url":"https://api.github.com/users/oni-link/gists{/gist_id}","starred_url":"https://api.github.com/users/oni-link/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oni-link/subscriptions","organizations_url":"https://api.github.com/users/oni-link/orgs","repos_url":"https://api.github.com/users/oni-link/repos","events_url":"https://api.github.com/users/oni-link/events{/privacy}","received_events_url":"https://api.github.com/users/oni-link/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/neovim/neovim/labels/RFC","name":"RFC","color":"dfdfdf"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":13,"created_at":"2015-11-08T11:30:13Z","updated_at":"2015-11-10T11:28:54Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/neovim/neovim/pulls/3630","html_url":"https://github.com/neovim/neovim/pull/3630","diff_url":"https://github.com/neovim/neovim/pull/3630.diff","patch_url":"https://github.com/neovim/neovim/pull/3630.patch"},"body":"eval.c: Protect saved funccalls against garbage collection\r\n\r\nWhen the current function call pointer is saved with a call to\r\nsave_funccal(), the funccall is not protected anymore against garbage\r\ncollection.\r\n\r\nTo fix this we collect a list of all saved but not yet restored\r\nfunccalls and tell the garbage collection not to free them.\r\n\r\nFixes #3588\r\n\r\n@ZyX-I , @tarruda"},"comment":{"url":"https://api.github.com/repos/neovim/neovim/issues/comments/155395279","html_url":"https://github.com/neovim/neovim/pull/3630#issuecomment-155395279","issue_url":"https://api.github.com/repos/neovim/neovim/issues/3630","id":155395279,"user":{"login":"tarruda","id":842846,"avatar_url":"https://avatars.githubusercontent.com/u/842846?v=3","gravatar_id":"","url":"https://api.github.com/users/tarruda","html_url":"https://github.com/tarruda","followers_url":"https://api.github.com/users/tarruda/followers","following_url":"https://api.github.com/users/tarruda/following{/other_user}","gists_url":"https://api.github.com/users/tarruda/gists{/gist_id}","starred_url":"https://api.github.com/users/tarruda/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tarruda/subscriptions","organizations_url":"https://api.github.com/users/tarruda/orgs","repos_url":"https://api.github.com/users/tarruda/repos","events_url":"https://api.github.com/users/tarruda/events{/privacy}","received_events_url":"https://api.github.com/users/tarruda/received_events","type":"User","site_admin":false},"created_at":"2015-11-10T11:28:54Z","updated_at":"2015-11-10T11:28:54Z","body":"> Should this diff also be included from your first patch?\r\n\r\nHere's my analysis of `ag s->toplevel`:\r\n\r\n```\r\nnvim/normal.c\r\n514:  if (s->toplevel && readbuf1_empty()) {\r\n839:    if (s->toplevel && readbuf1_empty()) {\r\n941:  if (curwin->w_p_scb && s->toplevel) {\r\n946:  if (curwin->w_p_crb && s->toplevel) {\r\n1053:  if (s->toplevel && readbuf1_empty()) {\r\n1373:  may_garbage_collect = s->toplevel;\r\n```\r\n\r\nAt first sight it may seem fine to remove the second argument of `normal_cmd` and set s->toplevel to false because `:normal` can't execute events, but three of these conditions are used for setting `v:count` in expression mappings(`: :map-expression`) and `:normal` can map commands(setting s->toplevel would break v:count for expression mappings reached from `:normal`). The other two conditions are used for scrollbind/cursorbind which I think are fine to ignore while in `:normal`.\r\n\r\nThis does bring another issue: mappings can execute arbitrary functions which could call `garbagecollect()`. This could result in garbage collection in the next vgetc call while processing `:normal`, and consequently trigger the use-after-free bug. Another possible issue can arise if it is possible for an expression mapping to force a `os_inchar` call(get characters from the user) bypassing the typeahead buffers(possibly through `f_confirm`?).\r\n\r\nMy guess is that the `toplevel` parameter in `normal_cmd` is being reused for too many things, we may need to introduce another flag to `NormalState` that controls if `v:count` is set independently from `s->toplevel`.\r\n\r\nI could be missing something though. @ZyX-I what are your thoughts?\r\n"}}