Arrays leak out of non-forked subshells #88
Comments
McDutchie
added
bug
|
That would be fine with me.
|
|
|
|
Through a process of systematic elimination of differences, I was able to determine what between 2012-02-29 and 2012-05-18 made the difference. It is the addition of these two lines: ksh/src/cmd/ksh93/bltins/typeset.c Lines 673 to 674 in 8c16f38 In other words, the original reproducer, which was basically unset qq; (typeset -A qq); (typeset -a qq)"succeeded" because ksh 2012-02-29 and older failed to fail; subshells were just as leaky. A better reproducer is: unset qq; (typeset -A qq); typeset -p qqIf the output is |
|
Adding an assignment value, even an empty array ( (empty output) |
|
The command unset qq; (typeset -A qq); typeset | grep -Fx "associative array qq"With that, the bug is confirmed present on 2002-06-28, and we can give up on hoping there being a bugfix to backport. |
McDutchie
changed the title
|
Indexed arrays are leaky in the same way: …and again, using |
|
I think I found the problem, using that workaround as a clue. For array declarations without an assignment, this call to ksh/src/cmd/ksh93/bltins/typeset.c Lines 696 to 699 in 8c16f38 The following diff fixes the reproducer for me; please test. diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 906d0a7..7c03078 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -693,10 +693,8 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
}
if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
{
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
+ if(comvar || iarray || (flag&NV_ARRAY) || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
_nv_unset(np,0);
-}
}
if(troot==shp->var_tree)
{Unfortunately, this diff causes one regression test failure: …but that will have to wait as the daystar is rising. Goodnight. |
|
The regression test failure can be fixed by checking if the array is empty with --- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -693,10 +693,9 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
}
if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
{
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
+ if(comvar || ((iarray || (nvflags&NV_ARRAY)) && nv_isnull(np)) || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC))
+ || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
_nv_unset(np,0);
-}
}
if(troot==shp->var_tree)
{I'll note that the regression test failure for associative arrays also applies to indexed arrays, so # This should be added to arrays2.sh, since the regression test failure for
# associative arrays was also occurring with indexed arrays after `typeset -a`.
typeset foo=bar
typeset -a foo
if [[ ${foo[0]} != bar ]]; then
err_exit $'initial value of $foo not preserved after \'typeset -a\''
fi |
|
Thanks, that works. This is also a nice chance to make that whole tangled mess of parentheses more readable. Sometimes it's just counterproductive to make the precedence of diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 906d0a7..cc1a799 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -691,13 +691,28 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
if(!comvar && !iarray)
continue;
}
- if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
- {
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
- _nv_unset(np,0);
-}
- }
+
+ /* Determine if we need to create a null value in the current scope using _nv_unset(). */
+ if(!nv_isarray(np) &&
+ !strchr(name,'=') &&
+ !(shp->envlist && nv_onlist(shp->envlist,name)) &&
+ (
+ /* compound variable? */
+ comvar ||
+ /* indexed or associative array that is initialized as null? */
+ (iarray || (nvflags&NV_ARRAY)) && nv_isnull(np) ||
+ /* variable exists in parent of virtual subshell? [EDIT: this comment is wrong] */
+ shp->last_root==shp->var_tree &&
+ (
+ tp->tp ||
+ !shp->st.real_fun && (nvflags&NV_STATIC) ||
+ !(flag&(NV_EXPORT|NV_RDONLY)) &&
+ nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)
+ )
+ )
+ )
+ _nv_unset(np,0);
+
if(troot==shp->var_tree)
{
if(iarray)I'm not quite sure what edit: Comment is not correct. Inserting a strategically placed debug warning and running the regression tests revealed that the whole block following that comment is for supporting type commands created by |
|
I'm experimenting further, learning to understand this code. It turns out that things appear to work just fine if we replace the entirety of the tests in the old initial if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))with a simple test for It's also quite logical, as this whole block decides whether a null scope needs to be created, which by definition should not be done for non-null nodes. (FYI, I figured out what these three deletable tests did: the first checks if it's a pre-existing array, the second checks if it's a normal argument in the form of an assignment, the third checks |
|
New diff: diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 906d0a7..723d5d2 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -691,13 +691,27 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
if(!comvar && !iarray)
continue;
}
- if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
- {
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
- _nv_unset(np,0);
-}
- }
+
+ /* Determine if we need to create a null value in the current scope using _nv_unset() */
+ if (
+ nv_isnull(np) && (
+ /* indexed array, or... */
+ iarray ||
+ /* associative array, or ... */
+ nvflags & NV_ARRAY ||
+ /* compound variable, or... */
+ comvar ||
+ /* 'Foo_t var', where Foo_t is type command generated by 'typeset -T' or 'enum'? */
+ shp->last_root == shp->var_tree && (
+ tp->tp ||
+ !shp->st.real_fun && (nvflags & NV_STATIC) ||
+ !(flag & (NV_EXPORT | NV_RDONLY)) &&
+ nv_isattr(np,(NV_EXPORT | NV_IMPORT)) == (NV_EXPORT | NV_IMPORT)
+ )
+ )
+ )
+ _nv_unset(np,0);
+
if(troot==shp->var_tree)
{
if(iarray) |
|
Following new regression test (for attributes.sh) reveals that two more attributes leak out of subshells for unset variables: # unset readonly exported variables
unset expect
typeset -A expect=(
[a]='typeset -x -r -a foo'
[b]='typeset -x -r -b foo'
[i]='typeset -x -r -i foo'
[l]='typeset -x -r -l foo'
[n]='typeset -n -x -r foo'
[s]='typeset -x -r -s -i 0 foo=0'
[u]='typeset -x -r -u foo'
[A]='typeset -x -r -A foo=()'
[E]='typeset -x -r -E foo'
[F]='typeset -x -r -F foo'
[H]='typeset -x -r -H foo'
[L]='typeset -x -r -L 0 foo'
[R]='typeset -x -r -R 0 foo'
[X]='typeset -x -r -X 32 foo'
[S]='typeset -x -r foo'
[T]='typeset -x -r foo'
[Z]='typeset -x -r -Z 0 -R 0 foo'
)
for flag in a b i l n s u A E F H L R X S T Z
do unset foo
actual=$(
(typeset "-$flag" foo; readonly foo; export foo; typeset -p foo)
typeset -p foo # check that no attribute leaked out of subshell
)
if [[ $actual != "${expect[$flag]}" ]]
then err_exit "unset readonly exported variable of type -$flag:" \
"expected $(printf %q "${expect[$flag]}"), got $(printf %q "$actual")"
fi
done
unset expect |
|
More experimentation seems to reveal that all of the tests in that jumbled mess of parentheses are redundant if we just check for two things: no value was assigned and there are no pre-existing attributes (such as Which means we can (hopefully) simplify it to the following diff: diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 906d0a7..743c2db 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -691,13 +691,11 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
if(!comvar && !iarray)
continue;
}
- if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
- {
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
- _nv_unset(np,0);
-}
- }
+
+ /* No value or attribute assigned? Create null value in current scope */
+ if(nv_isnull(np) && !nv_isattr(np,~0))
+ _nv_unset(np,0);
+
if(troot==shp->var_tree)
{
if(iarray)…and this also fixes the subshell leaks of both array attributes as well as the |
|
Ah, but sadly it is not that simple... With the above diff, if an attribute exists in the parent shell, the leak returns. |
I can get that leak to go away by replacing --- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -691,13 +691,11 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
if(!comvar && !iarray)
continue;
}
- if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
- {
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
- _nv_unset(np,0);
-}
- }
+
+ /* No value assigned? Create null value in current scope */
+ if(nv_isnull(np) && !(flag & (NV_EXPORT | NV_RDONLY)))
+ _nv_unset(np,0);
+
if(troot==shp->var_tree)
{
if(iarray) |
That works to fix the leak. However, a pre-existing export flag is always cleared, which is not good: (output should be Of course, clearing pre-existing attributes is expected behaviour for According to my testing, the following appears to work 100%, fixing all the leaks without clearing any pre-existing attributes: diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 906d0a7..d917506 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -691,13 +691,22 @@ static int setall(char **argv,register int flag,Dt_t *troot,struct tdata *tp
if(!comvar && !iarray)
continue;
}
- if(!nv_isarray(np) && !strchr(name,'=') && !(shp->envlist && nv_onlist(shp->envlist,name)))
+
+ /* Create local scope for virtual subshell */
+ if(shp->subshell)
{
- if(comvar || (shp->last_root==shp->var_tree && (tp->tp || (!shp->st.real_fun && (nvflags&NV_STATIC)) || (!(flag&(NV_EXPORT|NV_RDONLY)) && nv_isattr(np,(NV_EXPORT|NV_IMPORT))==(NV_EXPORT|NV_IMPORT)))))
-{
- _nv_unset(np,0);
-}
+ if(!nv_isattr(np,NV_NODISC|NV_ARRAY) && !nv_isvtree(np))
+ {
+ /*
+ * Variables with internal trap/discipline functions (LC_*, LINENO, etc.) need to be
+ * cloned, as moving them will remove the discipline function.
+ */
+ np=sh_assignok(np,2);
+ }
+ else
+ np=sh_assignok(np,0);
}
+
if(troot==shp->var_tree)
{
if(iarray)New regression test for attributes.sh (which exposed another minor issue unrelated to this one, see the TODOs): # unset exported readonly variables, combined with all other possible attributes
typeset -A expect=(
[a]='typeset -x -r -a foo'
[b]='typeset -x -r -b foo'
[i]='typeset -x -r -i foo'
[i37]='typeset -x -r -i 37 foo'
[l]='typeset -x -r -l foo'
[n]='typeset -n -r foo'
[s]='typeset -x -r -s -i 0 foo=0'
[u]='typeset -x -r -u foo'
[A]='typeset -x -r -A foo=()'
[C]='typeset -x -r foo=()'
[E]='typeset -x -r -E foo'
[E12]='typeset -x -r -E 12 foo'
[F]='typeset -x -r -F foo'
[F12]='typeset -x -r -F 12 foo'
[H]='typeset -x -r -H foo'
[L]='typeset -x -r -L 0 foo'
# [L17]='typeset -x -r -L 17 foo' # TODO: outputs 'typeset -x -r -L 0 foo'
[Mtolower]='typeset -x -r -l foo'
[Mtoupper]='typeset -x -r -u foo'
[R]='typeset -x -r -R 0 foo'
# [R17]='typeset -x -r -R 17 foo' # TODO: outputs 'typeset -x -r -R 0 foo'
[X]='typeset -x -r -X 32 foo'
[X17]='typeset -x -r -X 17 foo'
[S]='typeset -x -r foo'
[T]='typeset -x -r foo'
[Z]='typeset -x -r -Z 0 -R 0 foo'
# [Z13]='typeset -x -r -Z 13 -R 13 foo' # TODO: outputs 'typeset -x -r -Z 0 -R 0 foo'
)
for flag in a b i i37 l n s u A C E E12 F F12 H L Mtolower Mtoupper R X X17 S T Z
do unset foo
actual=$(
redirect 2>&1
export foo
(typeset "-$flag" foo; readonly foo; typeset -p foo)
typeset +x foo # unexport
leak=${ typeset -p foo; }
[[ -n $leak ]] && print "SUBSHELL LEAK: $leak"
)
if [[ $actual != "${expect[$flag]}" ]]
then err_exit "unset exported readonly with -$flag:" \
"expected $(printf %q "${expect[$flag]}"), got $(printf %q "$actual")"
fi
done
unset expect |
Previous discussion and reproducer: att#416
A straightforward workaround/fix would be to fork when an associative array is declared in a subshell:
Before applying that fix/workaround I'd like to try to figure out if it's possible to implement a fix for the non-forking subshell mechanism. I don't really have time to dive deeply into this right now, so… any ideas, anyone?
The text was updated successfully, but these errors were encountered: