McDutchie
commented
2 years ago It looks like I just accidentally fixed this bug in 870ca92a0821775a400445126bc87ade6b203f94!
@JohnoKing, @hyenias, can you confirm?
Closed JohnoKing closed 2 years ago
McDutchie
commented
2 years ago It looks like I just accidentally fixed this bug in 870ca92a0821775a400445126bc87ade6b203f94!
@JohnoKing, @hyenias, can you confirm?
McDutchie
commented
2 years ago It's particularly the change to local_exports() in xec.c that fixed it. Reverting that reintroduces the bug.
That change was:
src/cmd/ksh93/sh/xec.c: local_exports():
- Fix a separate bug exporting attributes to a new ksh function
scope, which was previously masked by the other bug. The
attributes (nvflag) were copied *after* nv_putval()ing the value,
which is incorrect as the behaviour of nv_putval() is influenced
by the attributes. But here, we're copying the value too, so we
can simplify the whole function by using nv_clone() instead. This
may also fix other corner cases. (re: c1994b87)
diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 0bfe05370..228a9a6fa 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -3047,15 +3047,9 @@ pid_t sh_fork(int flags, int *jobid)
static void local_exports(register Namval_t *np, void *data)
{
register Namval_t *mp;
- register char *cp;
NOT_USED(data);
- if(nv_isarray(np))
- nv_putsub(np,NIL(char*),0);
- if((cp = nv_getval(np)) && (mp = nv_search(nv_name(np), sh.var_tree, NV_ADD|HASH_NOSCOPE)) && nv_isnull(mp))
- {
- nv_putval(mp, cp, 0);
- mp->nvflag = np->nvflag;
- }
+ if(!nv_isnull(np) && (mp = nv_search(nv_name(np), sh.var_tree, NV_ADD|HASH_NOSCOPE)) && nv_isnull(mp))
+ nv_clone(np,mp,0);
}
So, since I found the exact cause of the fix, I'm closing this. If someone still finds a problem, please comment and I'll reopen.
McDutchie
commented
2 years ago Actually, I should commit a regression test before closing this.
McDutchie
commented
2 years ago Actually, this error message…
/tmp/foo[9]: typ1: var11: only simple variables can be exportedIsn't that a bug, too? There is no attempt to export anything in that reproducer script.
McDutchie
commented
2 years ago There is still something about typeset -p .sh.type that causes that bug – something is setting the NV_EXPORT attribute where it shouldn't.
typeset -Ttyp1 typ1=(
function get {
.sh.value="'Sample'";
}
)
typeset -p .sh.type
typ1 var11Output:
namespace sh.type
{
typeset -r typ1='Sample'
}
foo[7]: typ1: var11: only simple variables can be exportedCommenting out the typeset -p .sh.type makes the spurious error go away.
McDutchie
commented
2 years ago The bug can be seen even more clearly this way:
typeset -Ttyp1 typ1=(
function get {
.sh.value="'Sample'";
}
)
typ1 var11
typeset -p .sh.type
typeset -p .sh.typeOutput:
namespace sh.type
{
typeset -r typ1='Sample'
}
namespace sh.type
{
typeset -x -r typ1='Sample'
}The second typeset -p .sh.type makes an -x attribute appear out of nowhere.
McDutchie
commented
2 years ago I found the lines that cause the problem. Removing these two lines makes the spurious -x attribute go away:
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1523,8 +1523,6 @@ static int print_namval(Sfio_t *file,register Namval_t *np,register int flag, st
print_value(file,np,tp);
return(0);
}
- if(nv_isvtree(np))
- nv_onattr(np,NV_EXPORT);
if(cp=nv_getval(np))
{
if(indent)But that does cause a couple of regressions:
test attributes begins at 2022-06-07+02:15:36
attributes.sh[466]: FAIL: typeset -pC does not list only compound variables
test attributes failed at 2022-06-07+02:15:36 with exit code 1 [ 164 tests 1 error ]
test comvar begins at 2022-06-07+02:15:59
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvar.sh[313]: eval: syntax error at line 1: `end of file' unexpected
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvar.sh[314]: [[ (: not found
comvar.sh[314]: FAIL: $x != $y with set | grep
test comvar failed at 2022-06-07+02:15:59 with exit code 1 [ 101 tests 1 error ]The following seems to fix the spurious -x attribute bug, as well as the spurious error message in the original reproducer, without causing regressions. For reasons I don't understand yet, the NV_EXPORT attribute needs to be on during the nv_getval(np) call, presumably so that a discipline function that it triggers does the right thing. But that doesn't mean it should linger around if it wasn't set to begin with, so turn it back off if it was off.
diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index f85a72eb2..2516330b8 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1432,6 +1432,7 @@ static int print_namval(Sfio_t *file,register Namval_t *np,register int flag, st
{
register char *cp;
int indent=tp->indent, outname=0, isfun;
+ char tempexport=0;
sh_sigcheck();
if(flag)
flag = '\n';
@@ -1523,9 +1524,15 @@ static int print_namval(Sfio_t *file,register Namval_t *np,register int flag, st
print_value(file,np,tp);
return(0);
}
- if(nv_isvtree(np))
+ if(nv_isvtree(np) && !nv_isattr(np,NV_EXPORT))
+ {
nv_onattr(np,NV_EXPORT);
- if(cp=nv_getval(np))
+ tempexport++;
+ }
+ cp=nv_getval(np);
+ if(tempexport)
+ nv_offattr(np,NV_EXPORT);
+ if(cp)
{
if(indent)
sfnputc(file,'\t',indent);Wow. Apparently, the NV_EXPORT attribute turns off indentation when outputting compound variables.
Normally:
$ ksh -c "typeset -C z=(foo=bar baz=quux); typeset -pC"
typeset -C z=(baz=quux;foo=bar)After removing the nv_onattr(np,NV_EXPORT) from print_namval:
$ arch/*/bin/ksh -c "typeset -C z=(foo=bar baz=quux); typeset -pC"
typeset -C z=(
baz=quux
foo=bar
)And this is where it's checked for: https://github.com/ksh93/ksh/blob/a1af93f32382dec17c83b84f98a133b0d425f548/src/cmd/ksh93/sh/nvtree.c#L1043
Well, that confirms that turning off the export attribute again after printing is the correct thing to do. But what an ugly hack, though. Again. And not a trace of an informative comment. Again. headdesk
This bug was originally reported at https://github.com/att/ast/issues/1297. Ksh will crash if
typeset -p .sh.typeis used after a type is instantiated:I ran the reproducer under ASan and initially got a buffer overflow crash caused by using
memcmpinstead ofstrncmp(a fix for that has been submitted in https://github.com/ksh93/ksh/pull/457). After that was dealt with, I got another buffer overflow crash innv_getval:The reproducer was also run under gdb without ASan enabled to produce this stacktrace:
For some reason
sh_scoped()is returning a null pointer forIFS, which is likely related to the buffer overflow in the previous ASan crash. The patch below works around the null pointer, but isn't a correct fix:Results from the script with the workaround patch applied: