Fix summary:
------------
Some commands and functions interpret user locks (:lockvar) or internal
locks (fixed / readonly vars as in the v: scope) differently than others
or ignore some of them, so that a locked variable could be changed or
deleted, or a variable that should be deletable is not deleted.
The following fixes make this more consistent with the docs in
':he :lockvar' and with other expectations.
1. Fix :unlet removing items in locked dictionaries and lists.
This also fixes user locks failing to protect a variable scope
against variable deletion, as in ':lockvar 1 b:'.
2. Fix :unlet not removing locked items in unlocked user dicts or
lists.
3. Fix extend() overwriting locked dictionary items.
4. Fix remove() deleting a fixed scope-level variable, such as
a:<var> and l:self, which may crash vim.
5. Fix filter() deleting a fixed scope-level variable, such as
l:self, which is likely to crash vim.
6. Fix extend() and map() overwriting a write-protected scope-level
variable, such as a:this_is_a_loooooooooong_parameter_name.
7. Review, but do not change the partial implementation around
internal flag DI_FLAGS_LOCK, which apparently was meant as a
means for the user to protect scope-level variables individually
against deletion.
Introduction:
-------------
According to ':he :lockvar', a lock protects a variable only against
modification, but not against deletion from its holding list, dict or
variable scope. If the variable is a list or dict, then the lock
protects against addition and deletion of items to/from the list or
dictionary.
For fixed scopes (v: and a:) and fixed scope vars (l:self), vim
internally also uses the dict item flags DI_FLAGS_{FIX,RO,RO_SBX} to
protect individual scope-level variables against deletion (FIX) or
against change and deletion (RO, RO_SBX).
The problematic commands or functions can do one or more of the
following:
- crash vim
- ignore an internal lock (DI_FLAGS_{FIX,RO,RO_SBX})
- ignore a user lock (:lockvar)
- misinterpret a modification lock as a deletion lock.
:unlet and list/dict functions check different locks on 'fixed' scope or
user dicts and items:
dict/list head dict item(s) dict/list item typeval(s)
v: VAR_FIXED DI_FLAGS_FIX ... -
a: - DI_FLAGS_FIX / _RO VAR_FIXED
l:self - DI_FLAGS_FIX + _RO -
user: VAR_LOCKED / - - VAR_LOCKED / -
^------------------^------- :unlet
^--------------------------------------^------- remove() ...
:unlet appears to have been written with fixed scope-level variables in
mind, while list/dict functions like remove(), extend(), filter() and
map() appear to have been written with locks on user lists in mind (same
as for the list-only functions add(), insert(), reverse() and sort()).
Fix details:
------------
1. :unlet removes items in locked dictionaries and lists, while remove()
does not. According to ':he :lockvar', the behavior of remove() is
correct:
[depth: 1]
Lock the |List| or |Dictionary| itself, cannot add or remove
items, but can still change their values.
Example:
:let d = {'a': 99, 'b': 100}
:lockvar 1 d
:call remove(d, 'a')
E741: Value is locked: remove() argument
:unlet d.a
:let d
d {'b': 100}
Fix: make :unlet check locks on dicts and lists.
This also fixes the problem that the user cannot protect a variable
scope against variable deletion (as in ':lockvar 1 b:').
Example:
:let b:testvar = 123
:lockvar 1 b:
:unlet b:testvar
:let b:testvar
E121: Undefined variable: b:testvar
2. :unlet does not remove locked items in unlocked user dicts or lists,
while remove() does. According to ':he :lockvar', the behavior of
remove() is correct:
Lock the internal variable {name}. Locking means that it can no
longer be changed (until it is unlocked). A locked variable can
be deleted:
[...]
[depth: 2]
Also lock the values, cannot change the items. [...]
Example:
:let d = {'a': 99, 'b': 100}
:lockvar d.a
:unlet d.a
E741: Value is locked: d.a
:call remove(d, 'a')
:let d
d {'b': 100}
Fix: remove the checks for dict item locks and list item locks from
:unlet.
3. extend() overwrites locked dict items, while filter() and map() do
not. According to ':he lockvar' (see 2. above), filter() and map()
are correct.
Example:
:let d = {'a': 99, 'b': 100}
:lockvar d.a
:call filter(d, 'v:key != "a"')
E741: Value is locked: filter() argument
:call map(d, 'v:val += 1')
E741: Value is locked: map() argument
:call extend(d, {'a': 123})
:let d
d {'a': 123, 'b': 100}
Fix: make extend() check locks on dict items.
4. remove() can delete a scope-level variable that is internally
protected against deletion, while :unlet cannot. Apparently, the
behavior of :unlet is correct.
remove() only checks for VAR_FIXED (and VAR_LOCKED) on the dict
header.
It correctly does not check for VAR_FIXED on dict items but
incorrectly ignores DI_FLAGS_{FIX,RO,RO_SBX} flags on dict
items.
The v: scope dict header is additionally protected with
VAR_FIXED against variable deletion, so remove() correctly
does not remove variables in v:. But the a: scope dict
header does not use VAR_FIXED, and the l: dict header cannot
use VAR_FIXED to protect l:self (as it would lock the whole
l: scope), so both the a:* variables and l:self can be
remove()d.
The exact effect depends on the vim version, the random contents
of the uninitialized scope dict lock, and on how the storage for
the scope dict var is allocated.
Example 1 (for vim before version 7.3.560):
:fun! Tfunc(param1)
: call remove(a:, 'param1')
: echo a:param1
:endfun
:call Tfunc('testval')
Error detected while processing function Tfunc:
line 1:
E741: Value is locked: remove() argument
testval
This is almost the expected behavior (only that E742 should have
been reported). But this result is only possible before 7.3.560,
and it is triggered by a bug in these vim versions: they do not
initialize the memory used for a scope's dict lock (except for
the v: scope), and in this case the lock randomly contained
VAR_LOCKED. A similar result occurs when VAR_FIXED was found in
that lock.
After 7.3.560 the lock is always cleared, and this result cannot
occur.
Example 2:
:fun! Tfunc(param1)
: call remove(a:, 'param1')
: echo a:param1
:endfun
:call Tfunc('testval')
Vim: Caught deadly signal ABRT
Vim: Finished.
Aborted
This always happens after 7.3.560, and it may happen before
7.3.560.
'param1' is put into a fixed table of variables, and remove(),
after ignoring the DI_FLAGS_FIX on it, deletes the dict item of
a:param1 and tries to free its memory, which is not at the start
of the allocated chunk, leading to the ABRT in my glibc.
Example 3:
:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
: call remove(a:, 'this_is_a_loooooooooong_parameter_name')
: echo a:this_is_a_loooooooooong_parameter_name
:endfun
:call Tfunc('testval')
Error detected while processing function Tfunc:
line 2:
E121: Undefined variable: a:this_is_a_loooooooooong_parameter_name
E15: Invalid expression: a:this_is_a_loooooooooong_parameter_name
This always happens after 7.3.560, and it may happen before
7.3.560.
'this_is_a_loooooooooong_parameter_name' is too long to be
stored in the fixed table of a: variables, and therefore it uses
separately allocated memory, which is then correctly free()d by
remove(), so no crash.
The removal of the variable should have been prevented by the
DI_FLAGS_RO flag, which was ignored by remove().
Fixes:
- Make remove() check DI_FLAGS_*, like :unlet does it.
- Split the DI_FLAGS_FIX flag into
DI_FLAGS_FIX - keep this scope-level variable (no :unlet or
remove() allowed)
DI_FLAGS_ALLOC - separately free() this dict item
Now DI_FLAGS_FIX can be set on a dict item independently of
DI_FLAGS_ALLOC. The latter indicates that the item's memory
must be free()d separately.
As long as DI_FLAGS_ALLOC is set exactly when the dict item
must be freed separately, this avoids a crash even in the
presence of bugs in the lock checking logic. Also, a fixed
variable can now always use DI_FLAGS_FIX and does not have to
fall back on using DI_FLAGS_RO, which also protects against
deletion.
It now becomes straightforward to use the correct flag for
each of the separate purposes: free() separately (ALLOC),
protect against deletion (FIX), protect against modification
(RO, RO_SBX). But note that RO, RO_SBX still also protect
against deletion.
5. Similarly to remove(), the list/dict function filter() can delete
scope-level variables that are internally protected against deletion.
But differently from remove(), filter() also checks the variable's
locks, so that it cannot delete an a:* scope variable, which uses
VAR_FIXED on each variable. It works on l:self though, which does not
use VAR_FIXED anywhere.
Example:
:fun! Tfunc() dict
: call filter(l:, 'v:key != "self"')
: let l:self
:endfun
:let d = {'data': [0, 1, 2, 3], 'func': function("Tfunc")}
:call d.func()
Vim: Caught deadly signal ABRT
Vim: Finished.
Aborted
Again, vim tries to free() the l:self dict item, which is in the
middle of a larger allocated chunk.
For a vim with a version below 7.3.560, an output like the following
is also possible, as explained above for remove():
Error detected while processing function Tfunc:
line 1:
E742: Cannot change value of filter() argument
l:self {'data': [0, 1, 2, 3], 'len':
function('Tfunc')}
Fix: make filter() check DI_FLAGS_*, like :unlet does it.
6. The functions extend() and map() can overwrite write-protected
scope-level variables.
These two functions do not actually remove a variable, but can at
most overwrite it. Therefore, they should check DI_FLAGS_RO*, but can
continue to ignore DI_FLAGS_FIX.
Example:
:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
: call extend(a:, {'this_is_a_loooooooooong_parameter_name': 1234})
: echo a:this_is_a_loooooooooong_parameter_name
:endfun
:call Tfunc('testval')
1234
Again, the dict item for the long parameter name is allocated
separately and only DI_FLAGS_RO is set for it (as DI_FLAGS_FIX would
imply that this dict item is not allocated separately).
Still, extend() can overwrite the write-protected variable.
Fix: make extend() and map() check DI_FLAGS_RO*.
7. The flag DI_FLAGS_LOCK was apparently intended to provide
user-specified deletion locks for individual scope-level variables,
in analogy to the internal DI_FLAGS_FIX.
These individual variable deletion locks go beyond the model
documented in ':he :lockvar' (see 1. above), which only describes
variable modification locks and item addition / deletion locks for
*all* items in a list or dict.
Currently, only setting and clearing of DI_FLAGS_LOCK in :(un)lockvar
and a check for the flag in islocked() are implemented.
The current code sets DI_FLAGS_LOCK together with VAR_LOCKED on
scope-level variables. But DI_FLAGS_LOCK is never used anywhere
except in islocked(), where it has the same effect as the
VAR_LOCKED that is also set on the variable.
To finish the implementation, a command syntax must be found which
sets deletion locks on individual variables (as opposed to the
modification locks set by ':lockvar <varname>').
But this is not the intention of this patch. It only aims at fixing
bugs with the documented user locks and with internal locks.
Fix: nothing. Leave the partial implementation as it is.
diff -r 39e174e02dec src/eval.c
--- a/src/eval.c Thu Apr 09 22:08:22 2015 +0200
+++ b/src/eval.c Sat Apr 11 19:23:21 2015 +0200
@@ -3658,7 +3658,8 @@ do_unlet_var(lp, name_end, forceit)
ret = FAIL;
*name_end = cc;
}
- else if (tv_check_lock(lp->ll_tv->v_lock, lp->ll_name))
+ else if ((lp->ll_list != NULL && tv_check_lock(lp->ll_list->lv_lock,
lp->ll_name))
+ || (lp->ll_dict != NULL && tv_check_lock(lp->ll_dict->dv_lock,
lp->ll_name)))
return FAIL;
else if (lp->ll_range)
{
@@ -3709,17 +3710,28 @@ do_unlet(name, forceit)
hashtab_T *ht;
hashitem_T *hi;
char_u *varname;
+ dict_T *d;
dictitem_T *di;
ht = find_var_ht(name, &varname);
if (ht != NULL && *varname != NUL)
{
+ if (ht == &globvarht)
+ d = &globvardict;
+ else if (current_funccal != NULL && ht ==
¤t_funccal->l_vars.dv_hashtab)
+ d = ¤t_funccal->l_vars;
+ else
+ {
+ di = find_var_in_ht(ht, *name, "", FALSE);
+ d = di->di_tv.vval.v_dict;
+ }
hi = hash_find(ht, varname);
if (!HASHITEM_EMPTY(hi))
{
di = HI2DI(hi);
if (var_check_fixed(di->di_flags, name)
- || var_check_ro(di->di_flags, name))
+ || var_check_ro(di->di_flags, name)
+ || tv_check_lock(d->dv_lock, name))
return FAIL;
delete_var(ht, hi);
return OK;
@@ -7269,7 +7281,7 @@ dictitem_alloc(key)
if (di != NULL)
{
STRCPY(di->di_key, key);
- di->di_flags = 0;
+ di->di_flags = DI_FLAGS_ALLOC;
}
return di;
}
@@ -7288,7 +7300,7 @@ dictitem_copy(org)
if (di != NULL)
{
STRCPY(di->di_key, org->di_key);
- di->di_flags = 0;
+ di->di_flags = DI_FLAGS_ALLOC;
copy_tv(&org->di_tv, &di->di_tv);
}
return di;
@@ -7320,7 +7332,8 @@ dictitem_free(item)
dictitem_T *item;
{
clear_tv(&item->di_tv);
- vim_free(item);
+ if (item->di_flags & DI_FLAGS_ALLOC)
+ vim_free(item);
}
/*
@@ -10481,6 +10494,7 @@ dict_extend(d1, d2, action)
dictitem_T *di1;
hashitem_T *hi2;
int todo;
+ char *arg_errmsg = N_("extend() argument");
todo = (int)d2->dv_hashtab.ht_used;
for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
@@ -10515,6 +10529,9 @@ dict_extend(d1, d2, action)
}
else if (*action == 'f' && HI2DI(hi2) != di1)
{
+ if (tv_check_lock(di1->di_tv.v_lock, (char_u *)_(arg_errmsg))
+ || var_check_ro(di1->di_flags, (char_u *)_(arg_errmsg)))
+ break;
clear_tv(&di1->di_tv);
copy_tv(&HI2DI(hi2)->di_tv, &di1->di_tv);
}
@@ -10851,6 +10868,10 @@ filter_map(argvars, rettv, map)
--todo;
di = HI2DI(hi);
if (tv_check_lock(di->di_tv.v_lock,
+ (char_u *)_(arg_errmsg))
+ || (!map && var_check_fixed(di->di_flags,
+ (char_u *)_(arg_errmsg)))
+ || var_check_ro(di->di_flags,
(char_u *)_(arg_errmsg)))
break;
vimvars[VV_KEY].vv_str = vim_strsave(di->di_key);
@@ -15819,7 +15840,8 @@ f_remove(argvars, rettv)
di = dict_find(d, key, -1);
if (di == NULL)
EMSG2(_(e_dictkey), key);
- else
+ else if (!var_check_fixed(di->di_flags, (char_u *)_(arg_errmsg))
+ && !var_check_ro(di->di_flags, (char_u
*)_(arg_errmsg)))
{
*rettv = di->di_tv;
init_tv(&di->di_tv);
@@ -21303,7 +21325,7 @@ vars_clear_ext(ht, free_val)
v = HI2DI(hi);
if (free_val)
clear_tv(&v->di_tv);
- if ((v->di_flags & DI_FLAGS_FIX) == 0)
+ if (v->di_flags & DI_FLAGS_ALLOC)
vim_free(v);
}
}
@@ -21502,7 +21524,7 @@ set_var(name, tv, copy)
vim_free(v);
return;
}
- v->di_flags = 0;
+ v->di_flags = DI_FLAGS_ALLOC;
}
if (copy || tv->v_type == VAR_NUMBER || tv->v_type == VAR_FLOAT)
@@ -23656,7 +23678,7 @@ call_user_func(fp, argcount, argvars, re
+ STRLEN(name)));
if (v == NULL)
break;
- v->di_flags = DI_FLAGS_RO;
+ v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX | DI_FLAGS_ALLOC;
}
STRCPY(v->di_key, name);
hash_add(&fc->l_avars.dv_hashtab, DI2HIKEY(v));
diff -r 39e174e02dec src/structs.h
--- a/src/structs.h Thu Apr 09 22:08:22 2015 +0200
+++ b/src/structs.h Sat Apr 11 19:23:21 2015 +0200
@@ -1203,10 +1203,11 @@ struct dictitem_S
typedef struct dictitem_S dictitem_T;
-#define DI_FLAGS_RO 1 /* "di_flags" value: read-only variable */
-#define DI_FLAGS_RO_SBX 2 /* "di_flags" value: read-only in the sandbox */
-#define DI_FLAGS_FIX 4 /* "di_flags" value: fixed variable, not allocated */
-#define DI_FLAGS_LOCK 8 /* "di_flags" value: locked variable */
+#define DI_FLAGS_RO 1 /* "di_flags" value: read-only variable */
+#define DI_FLAGS_RO_SBX 2 /* "di_flags" value: read-only in the sandbox */
+#define DI_FLAGS_FIX 4 /* "di_flags" value: fixed: no :unlet or remove() */
+#define DI_FLAGS_LOCK 8 /* "di_flags" value: locked variable */
+#define DI_FLAGS_ALLOC 16 /* "di_flags" value: separately allocated */
/*
* Structure to hold info about a Dictionary.
diff -r 39e174e02dec src/testdir/test55.in
--- a/src/testdir/test55.in Thu Apr 09 22:08:22 2015 +0200
+++ b/src/testdir/test55.in Sat Apr 11 19:23:21 2015 +0200
@@ -282,6 +282,142 @@ let l = [0, 1, 2, 3]
: $put =ps
: endfor
:endfor
+:"
+:" Unletting locked variables
+:$put ='Unletting:'
+:for depth in range(5)
+: $put ='depth is ' . depth
+: for u in range(3)
+: unlet l
+: let l = [0, [1, [2, 3]], {4: 5, 6: {7: 8}}]
+: exe "lockvar " . depth . " l"
+: if u == 1
+: exe "unlockvar l"
+: elseif u == 2
+: exe "unlockvar " . depth . " l"
+: endif
+: let ps =
islocked("l").islocked("l[1]").islocked("l[1][1]").islocked("l[1][1][0]").'-'.islocked("l[2]").islocked("l[2]['6']").islocked("l[2]['6'][7]")
+: $put =ps
+: let ps = ''
+: try
+: unlet l[2]['6'][7]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l[2][6]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l[2]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l[1][1][0]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l[1][1]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l[1]
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: try
+: unlet l
+: let ps .= 'p'
+: catch
+: let ps .= 'F'
+: endtry
+: $put =ps
+: endfor
+:endfor
+:"
+:" Locked variables and :unlet or list / dict functions
+:$put ='Locks and commands or functions:'
+:"
+:$put ='No :unlet after lock on dict:'
+:unlet! d
+:let d = {'a': 99, 'b': 100}
+:lockvar 1 d
+:try
+: unlet d.a
+: $put ='did :unlet'
+:catch
+: $put =v:exception[:16]
+:endtry
+:$put =string(d)
+:"
+:$put =':unlet after lock on dict item:'
+:unlet! d
+:let d = {'a': 99, 'b': 100}
+:lockvar d.a
+:try
+: unlet d.a
+: $put ='did :unlet'
+:catch
+: $put =v:exception[:16]
+:endtry
+:$put =string(d)
+:"
+:$put ='No extend() after lock on dict item:'
+:unlet! d
+:let d = {'a': 99, 'b': 100}
+:lockvar d.a
+:try
+: $put =string(extend(d, {'a': 123}))
+: $put ='did extend()'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:"
+:$put ='No remove() of write-protected scope-level variable:'
+:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
+: try
+: $put =string(remove(a:, 'this_is_a_loooooooooong_parameter_name'))
+: $put ='did remove()'
+: catch
+: $put =v:exception[:14]
+: endtry
+:endfun
+:call Tfunc('testval')
+:"
+:$put ='No extend() of write-protected scope-level variable:'
+:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
+: try
+: $put =string(extend(a:, {'this_is_a_loooooooooong_parameter_name': 1234}))
+: $put ='did extend()'
+: catch
+: $put =v:exception[:14]
+: endtry
+:endfun
+:call Tfunc('testval')
+:"
+:$put ='No :unlet of variable in locked scope:'
+:let b:testvar = 123
+:lockvar 1 b:
+:try
+: unlet b:testvar
+: $put ='b:testvar was :unlet: '. (!exists('b:testvar'))
+:catch
+: $put =v:exception[:16]
+:endtry
+:unlockvar 1 b:
+:unlet! b:testvar
+:"
:unlet l
:let l = [1, 2, 3, 4]
:lockvar! l
diff -r 39e174e02dec src/testdir/test55.ok
--- a/src/testdir/test55.ok Thu Apr 09 22:08:22 2015 +0200
+++ b/src/testdir/test55.ok Sat Apr 11 19:23:21 2015 +0200
@@ -86,6 +86,58 @@ 0011-011
FFpFFpp
0000-000
ppppppp
+Unletting:
+depth is 0
+0000-000
+ppppppp
+0000-000
+ppppppp
+0000-000
+ppppppp
+depth is 1
+1000-000
+ppFppFp
+0000-000
+ppppppp
+0000-000
+ppppppp
+depth is 2
+1100-100
+pFFpFFp
+0000-000
+ppppppp
+0000-000
+ppppppp
+depth is 3
+1110-110
+FFFFFFp
+0010-010
+FppFppp
+0000-000
+ppppppp
+depth is 4
+1111-111
+FFFFFFp
+0011-011
+FppFppp
+0000-000
+ppppppp
+Locks and commands or functions:
+No :unlet after lock on dict:
+Vim(unlet):E741:
+{'a': 99, 'b': 100}
+:unlet after lock on dict item:
+did :unlet
+{'b': 100}
+No extend() after lock on dict item:
+Vim(put):E741:
+{'a': 99, 'b': 100}
+No remove() of write-protected scope-level variable:
+Vim(put):E795:
+No extend() of write-protected scope-level variable:
+Vim(put):E742:
+No :unlet of variable in locked scope:
+Vim(unlet):E741:
[1, 2, 3, 4]
[1, 2, 3, 4]
[1, 2, 3, 4]
--
Olaf Dabrunz (oda <at> fctrace.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.
