Permalink
Browse files

patch 8.0.0344: unlet command leaks memory

Problem:    Unlet command leaks memory. (Nikolai Pavlov)
Solution:   Free the memory on error. (closes #1497)
  • Loading branch information...
brammool committed Feb 20, 2017
1 parent e7877fe commit 49439c4cdf7d2822255f292adda4226656fe144d
Showing with 9 additions and 0 deletions.
  1. +3 −0 src/eval.c
  2. +4 −0 src/testdir/test_unlet.vim
  3. +2 −0 src/version.c
View
@@ -2079,7 +2079,10 @@ get_lval(
}
/* existing variable, need to check if it can be changed */
else if (var_check_ro(lp->ll_di->di_flags, name, FALSE))
+ {
+ clear_tv(&var1);

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Feb 20, 2017

I see that other clear_tv(&var1) occurrences are guarded by if (len == -1) and I have a similar fix, but with this guard. Are you sure that this code is valid? I failed to make it crash (or make asan report uninitialized memory access) with unlet v:.count (which should have len set to something bigger), but I did not check a release build. And it looks like there is uninitialized memory access:

  • var1 at line 1834 is not initialized
  • first time it is mentioned in else branch starting at 1931, first mention at 1940. But according to the condition on 1918 for unlet v:.count this branch is not entered
  • outside of that branch before this place var1 is only passed to clear_tv() (2019, 2067, 2075) and get_tv_string_chk() (2016), neither function may initialize var1 and all occurrences are guarded by if (len == -1)
  • occurences below are by no means better: clear_tv() (2088, 2102), get_tv_number() (2100).
@ZyX-I

ZyX-I Feb 20, 2017

I see that other clear_tv(&var1) occurrences are guarded by if (len == -1) and I have a similar fix, but with this guard. Are you sure that this code is valid? I failed to make it crash (or make asan report uninitialized memory access) with unlet v:.count (which should have len set to something bigger), but I did not check a release build. And it looks like there is uninitialized memory access:

  • var1 at line 1834 is not initialized
  • first time it is mentioned in else branch starting at 1931, first mention at 1940. But according to the condition on 1918 for unlet v:.count this branch is not entered
  • outside of that branch before this place var1 is only passed to clear_tv() (2019, 2067, 2075) and get_tv_string_chk() (2016), neither function may initialize var1 and all occurrences are guarded by if (len == -1)
  • occurences below are by no means better: clear_tv() (2088, 2102), get_tv_number() (2100).

This comment has been minimized.

Show comment
Hide comment
@brammool

brammool Feb 23, 2017

Contributor
@brammool

brammool via email Feb 23, 2017

Contributor
return NULL;
+ }
if (len == -1)
clear_tv(&var1);
@@ -17,3 +17,7 @@ func Test_not_existing()
unlet! does_not_exist
call assert_fails('unlet does_not_exist', 'E108:')
endfunc
+
+func Test_unlet_fails()
+ call assert_fails('unlet v:["count"]', 'E46:')
+endfunc
View
@@ -764,6 +764,8 @@ static char *(features[]) =
static int included_patches[] =
{ /* Add new patch number below this line */
+/**/
+ 344,
/**/
343,
/**/

0 comments on commit 49439c4

Please sign in to comment.