Decide what to do about the unknown getn discipline
#435
Comments
|
The following thread on the old mailing list seems relevant: |
|
There is a difference when referencing numeric values within arithmetic expressions as compared to using their parameter substitutions besides being faster--namely precision. So, perhaps these get/getn disciplines allow someone when working with math to code using full precision (or a different level of precision/rounding/etc.) as compared to outside of arithmetic expressions, maybe performing formatting equal to or greater than what can be accomplished by typeset's option combinations. Reference #353 |
|
As what happens with $KSH_VERSION, it demonstrates some of this flexible behavior benefitting the programmer. |
Wait, it appears I was completely wrong about the 'no fallback' part. Sorry. Well, that does change things. |
|
While experimenting with $ cat /tmp/a
foo=one
foo.getn()
{
.sh.value=1
}
foo.get()
{
.sh.value=one
}
unset -f foo.getn
$ ENV=/dev/null gdb arch/linux.i386-64/bin/ksh
GNU gdb (GDB) 11.2
[Redacted unnecessary info]
Reading symbols from arch/linux.i386-64/bin/ksh...
(gdb) run
Starting program: /home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh
$ . /tmp/a
$ echo $((foo))
/home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh: one: parameter not set
$ set -o emacs
$ [Detaching after vfork from child process 2315287]
. /tmp/a
Program received signal SIGSEGV, Segmentation fault.
nv_arrayptr (np=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:929
929 if(nv_isattr(np,NV_ARRAY))
(gdb) bt
#0 nv_arrayptr (np=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:929
#1 0x000055555557813e in nv_putsub (np=0x0, sp=0x0, mode=268435456) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:1179
#2 0x000055555556922d in block_done (bp=0x7fffffffcc10) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:215
#3 0x0000555555569e42 in assign (np=0x55555572ec70, val=0x555555703585 "one", flags=0, handle=0x55555572ed30) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:371
#4 0x0000555555569082 in nv_putv (np=0x55555572ec70, value=0x555555703585 "one", flags=0, nfp=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:145
#5 0x00005555555a52d7 in nv_putval (np=0x55555572ec70, string=0x555555703585 "one", flags=0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1631
#6 0x00005555555a4fa9 in nv_open (name=0x555555703581 "foo=one", root=0x555555706720, flags=131584) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1550
#7 0x00005555555a1f97 in nv_setlist (arg=0x555555703570, flags=131584, typ=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:612
#8 0x00005555555c4549 in sh_exec (t=0x0, flags=4) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1117
#9 0x00005555555c75d6 in sh_exec (t=0x0, flags=4) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2079
#10 0x00005555555c3128 in sh_eval (iop=0x0, mode=0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:717
#11 0x00005555555dba9d in b_dot_cmd (n=0, argv=0x555555703338, context=0x5555556eab10 <sh+1456>) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/misc.c:324
#12 0x00005555555c5440 in sh_exec (t=0x0, flags=4) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1366
#13 0x00005555555684a5 in exfile (iop=0x0, fno=0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:591
#14 0x00005555555679d9 in sh_main (ac=1, av=0x7fffffffe508, userinit=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:352
#15 0x0000555555566dbe in main (argc=1, argv=0x7fffffffe508) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
(gdb) p np
$1 = (Namval_t *) 0x0
(gdb) frame 2
#2 0x000055555556922d in block_done (bp=0x7fffffffcc10) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:215
215 nv_putsub(bp->np, bp->sub,(bp->isub<0?0:bp->isub)|ARRAY_SETSUB);
(gdb) p bp->np
$2 = (Namval_t *) 0x0 |
|
So nv_putsub() is called from block_done() with np==NULL. Seems easy enough to prevent that: --- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -211,7 +211,7 @@ static struct blocked *block_info(Namval_t *np, struct blocked *pp)
static void block_done(struct blocked *bp)
{
blist = bp = bp->next;
- if(bp && (bp->isub>=0 || bp->sub))
+ if(bp && (bp->isub>=0 || bp->sub) && bp->np)
nv_putsub(bp->np, bp->sub,(bp->isub<0?0:bp->isub)|ARRAY_SETSUB);
}
That still leaves problems though. After that patch, a third |
|
I ran the reproducer under ASan and it crashes with a buffer overflow (when run with and without the above patch applied). |
|
I don't understand the cause of that last crash. The backtrace suggests it's in the middle of performing a tilde expansion (tilde_expand2()) but no tilde expansion happens in the reproducer. I was able to reproduce the first crash with ASan on clang. With the patch below it does not crash any more. The change is that diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index 725ff079e..c6ad4d50b 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -211,8 +211,13 @@ static struct blocked *block_info(Namval_t *np, struct blocked *pp)
static void block_done(struct blocked *bp)
{
blist = bp = bp->next;
- if(bp && (bp->isub>=0 || bp->sub))
- nv_putsub(bp->np, bp->sub,(bp->isub<0?0:bp->isub)|ARRAY_SETSUB);
+ if(bp)
+ {
+ if(!bp->np)
+ blist = NIL(struct blocked *);
+ else if(bp->isub>=0 || bp->sub)
+ nv_putsub(bp->np, bp->sub,(bp->isub<0?0:bp->isub)|ARRAY_SETSUB);
+ }
}
/* |
|
More crashing (including after applying the patch above): Backtrace: |
|
This is clearly still experimental and nowhere near ready for release, so I'm removing the getn discipline from the 1.0 branch. I'll leave it in the dev branch in hopes of fixing it. |
|
On the other hand, that crash is quite easy to patch – it's a straightforward failure to check for a non-null pointer. Maybe I won't remove it from 1.0 just yet. --- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -367,7 +367,7 @@ static void assign(Namval_t *np,const char* val,int flags,Namfun_t *handle)
done:
if(bp== &block)
block_done(bp);
- if(nq && nq->nvalue.rp->running==1)
+ if(nq && nq->nvalue.rp && nq->nvalue.rp->running==1)
{
nq->nvalue.rp->running=0;
_nv_unset(nq,0); |
The 'getn' discipline is experimental and undocumented, the only mention of it being an old mailing list post from David Korn: https://www.mail-archive.com/[email protected]/msg00601.html But it still should not crash. $ LC_NUMERIC=C ENV=/./dev/null arch/*/bin/ksh $ foo.getn() { .sh.value=2.3*4.5; } $ typeset -F foo Memory fault src/cmd/ksh93/sh/nvdisc.c: assign(): - Check that the nvalue union has a non-NULL pointer before using it. Progresses: #435
The 'getn' discipline is experimental and undocumented, the only mention of it being an old mailing list post from David Korn: https://www.mail-archive.com/[email protected]/msg00601.html But it still should not crash. $ LC_NUMERIC=C ENV=/./dev/null arch/*/bin/ksh $ foo.getn() { .sh.value=2.3*4.5; } $ typeset -F foo Memory fault src/cmd/ksh93/sh/nvdisc.c: assign(): - Check that the nvalue union has a non-NULL pointer before using it. Progresses: #435
|
Here is a patch for removing the Patch to remove `getn` discipline--- a/src/cmd/ksh93/data/variables.c
+++ b/src/cmd/ksh93/data/variables.c
@@ -108,7 +108,7 @@ const struct shtable2 shtab_variables[] =
"", 0, (char*)0
};
-const char *nv_discnames[] = { "get", "set", "append", "unset", "getn", 0 };
+const char *nv_discnames[] = { "get", "set", "append", "unset", 0 };
#if SHOPT_STATS
const Shtable_t shtab_stats[] =
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -157,13 +157,12 @@ void nv_putv(Namval_t *np, const char *value, int flags, register Namfun_t *nfp)
#define ASSIGN 1
#define APPEND 2
#define UNASSIGN 3
-#define LOOKUPN 4
#define BLOCKED ((Namval_t*)&nv_local)
struct vardisc
{
Namfun_t fun;
- Namval_t *disc[5];
+ Namval_t *disc[4];
};
struct blocked
@@ -378,15 +377,15 @@ done:
* This function executes a lookup disc and then performs
* the lookup on the given node <np>
*/
-static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
+static char* lookup(Namval_t *np, Sfdouble_t *dp,Namfun_t *handle)
{
register struct vardisc *vp = (struct vardisc*)handle;
struct blocked block, *bp = block_info(np, &block);
- register Namval_t *nq = vp->disc[type];
+ register Namval_t *nq = vp->disc[LOOKUPS];
register char *cp=0;
Namval_t node;
union Value *up = np->nvalue.up;
- if(nq && !isblocked(bp,type))
+ if(nq && !isblocked(bp,LOOKUPS))
{
struct checkpt checkpoint;
int jmpval;
@@ -401,12 +400,7 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
nv_onattr(SH_VALNOD,NV_NOFREE);
_nv_unset(SH_VALNOD,0);
}
- if(type==LOOKUPN)
- {
- nv_onattr(SH_VALNOD,NV_DOUBLE|NV_INTEGER);
- nv_setsize(SH_VALNOD,10);
- }
- block(bp,type);
+ block(bp,LOOKUPS);
block(bp, UNASSIGN); /* make sure nv_setdisc doesn't invalidate 'vp' by freeing it */
sh_pushcontext(&checkpoint, 1);
jmpval = sigsetjmp(checkpoint.buff, 0);
@@ -416,15 +410,10 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
if(sh.topfd != checkpoint.topfd)
sh_iorestore(checkpoint.topfd, jmpval);
unblock(bp,UNASSIGN);
- unblock(bp,type);
- if(!vp->disc[type])
+ unblock(bp,LOOKUPS);
+ if(!vp->disc[LOOKUPS])
chktfree(np,vp);
- if(type==LOOKUPN)
- {
- cp = (char*)(SH_VALNOD->nvalue.cp);
- *dp = nv_getnum(SH_VALNOD);
- }
- else if(cp = nv_getval(SH_VALNOD))
+ if(cp = nv_getval(SH_VALNOD))
cp = stkcopy(stkstd,cp);
_nv_unset(SH_VALNOD,NV_RDONLY);
if(!nv_isnull(&node))
@@ -438,12 +427,7 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
if(nv_isarray(np))
np->nvalue.up = up;
if(!cp)
- {
- if(type==LOOKUPS)
- cp = nv_getv(np,handle);
- else
- *dp = nv_getn(np,handle);
- }
+ cp = nv_getv(np,handle);
if(bp== &block)
block_done(bp);
if(nq && nq->nvalue.rp && nq->nvalue.rp->running==1)
@@ -456,14 +440,7 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
static char* lookups(Namval_t *np, Namfun_t *handle)
{
- return(lookup(np,LOOKUPS,(Sfdouble_t*)0,handle));
-}
-
-static Sfdouble_t lookupn(Namval_t *np, Namfun_t *handle)
-{
- Sfdouble_t d;
- lookup(np,LOOKUPN, &d ,handle);
- return(d);
+ return(lookup(np,(Sfdouble_t*)0,handle));
}
@@ -561,10 +538,7 @@ char *nv_setdisc(register Namval_t* np,register const char *event,Namval_t *acti
else if(action)
{
Namdisc_t *dp = (Namdisc_t*)vp->fun.disc;
- if(type==LOOKUPS)
- dp->getval = lookups;
- else if(type==LOOKUPN)
- dp->getnum = lookupn;
+ dp->getval = lookups;
vp->disc[type] = action;
}
else |
The patch is not actually sufficient. When I compile ksh with ASan and that patch, and replicate the reproducer, then a very similar crash occurs, intermittently, with a non-null pointer that is invalid. This is all taking much more of my time than it's actually worth. :/ |
|
Considering that:
I now think it's best to just get rid of the whole thing. I'm also not actually convinced this is the best design. Defining additional disciplines will confuse most users and a correct usage in scripts will be the exception rather than the rule. The problem it was meant to solve could be perhaps be solved in a different way. In the normal |
*.getn discipline functions cause .sh.value to have a float type for arithmetic expressions that get the value of foo, avoiding the problem of having to convert between floats and strings (e.g. rounding errors). There is no corresponding .setn discipline. A search in the ast-open-archive repo reveals that the getn discipline was quietly added in version 2009-08-21 93t+, with not even a mention in the RELEASE file. The one available mention on the internet is this old thread: https://www.mail-archive.com/[email protected]/msg00601.html Apparently a setn discipline *was* planned, but never implemented. getn discipline functions may also crash in several ways. I've been unsuccessful at solving all the crashes, particularly as one of them is intermittent. This should not be in the 1.0 release. Further discussion: #435
|
So, getn is now gone from the 1.0 branch. I'm leaving it on dev for now. I've found a relatively simple way to fold the getn discipline into the regular get discipline. The patch below is against the dev branch. When applied, if you define a This seems much more logical to me than a separate getn discipline. But it's a (small) incompatibility that could conceivably require some script to be changed, so it's definitely for a post-1.0 release. @hyenias, would you like to test this? diff --git a/src/cmd/ksh93/data/variables.c b/src/cmd/ksh93/data/variables.c
index a4b061b65..17acfc83b 100644
--- a/src/cmd/ksh93/data/variables.c
+++ b/src/cmd/ksh93/data/variables.c
@@ -109,7 +109,7 @@ const struct shtable2 shtab_variables[] =
"", 0, (char*)0
};
-const char *nv_discnames[] = { "get", "set", "append", "unset", "getn", 0 };
+const char *nv_discnames[] = { "get", "set", "append", "unset", 0 };
#if SHOPT_STATS
const Shtable_t shtab_stats[] =
diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index b0e3b5849..de5a04eee 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -154,16 +154,15 @@ void nv_putv(Namval_t *np, const char *value, int flags, register Namfun_t *nfp)
}
}
-#define LOOKUPS 0
+#define LOOKUP 0
#define ASSIGN 1
#define APPEND 2
#define UNASSIGN 3
-#define LOOKUPN 4
struct vardisc
{
Namfun_t fun;
- Namval_t *disc[5];
+ Namval_t *disc[4];
};
struct blocked
@@ -290,8 +289,8 @@ static void assign(Namval_t *np,const char* val,int flags,Namfun_t *handle)
savelex = *lexp;
sh_lexopen(lexp, 0); /* needs full init (0), not what it calls reinit (1) */
block(bp,type);
- if(bflag = (type==APPEND && !isblocked(bp,LOOKUPS)))
- block(bp,LOOKUPS);
+ if(bflag = (type==APPEND && !isblocked(bp,LOOKUP)))
+ block(bp,LOOKUP);
sh_pushcontext(&checkpoint, 1);
jmpval = sigsetjmp(checkpoint.buff, 0);
if(!jmpval)
@@ -301,7 +300,7 @@ static void assign(Namval_t *np,const char* val,int flags,Namfun_t *handle)
sh_iorestore(checkpoint.topfd, jmpval);
unblock(bp,type);
if(bflag)
- unblock(bp,LOOKUPS);
+ unblock(bp,LOOKUP);
if(!vp->disc[type])
chktfree(np,vp);
*lexp = savelex;
@@ -377,16 +376,17 @@ done:
/*
* This function executes a lookup disc and then performs
* the lookup on the given node <np>
+ * If <dp> is non-NULL, a numeric lookup is performed
*/
-static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
+static char* lookup(Namval_t *np, Sfdouble_t *dp, Namfun_t *handle)
{
register struct vardisc *vp = (struct vardisc*)handle;
struct blocked block, *bp = block_info(np, &block);
- register Namval_t *nq = vp->disc[type];
+ register Namval_t *nq = vp->disc[LOOKUP];
register char *cp=0;
Namval_t node;
union Value *up = np->nvalue.up;
- if(nq && !isblocked(bp,type))
+ if(nq && !isblocked(bp,LOOKUP))
{
struct checkpt checkpoint;
int jmpval;
@@ -401,12 +401,12 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
nv_onattr(SH_VALNOD,NV_NOFREE);
_nv_unset(SH_VALNOD,0);
}
- if(type==LOOKUPN)
+ if(dp)
{
nv_onattr(SH_VALNOD,NV_DOUBLE|NV_INTEGER);
nv_setsize(SH_VALNOD,10);
}
- block(bp,type);
+ block(bp,LOOKUP);
block(bp, UNASSIGN); /* make sure nv_setdisc doesn't invalidate 'vp' by freeing it */
sh_pushcontext(&checkpoint, 1);
jmpval = sigsetjmp(checkpoint.buff, 0);
@@ -416,10 +416,10 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
if(sh.topfd != checkpoint.topfd)
sh_iorestore(checkpoint.topfd, jmpval);
unblock(bp,UNASSIGN);
- unblock(bp,type);
- if(!vp->disc[type])
+ unblock(bp,LOOKUP);
+ if(!vp->disc[LOOKUP])
chktfree(np,vp);
- if(type==LOOKUPN)
+ if(dp)
{
cp = (char*)(SH_VALNOD->nvalue.cp);
*dp = nv_getnum(SH_VALNOD);
@@ -439,10 +439,10 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
np->nvalue.up = up;
if(!cp)
{
- if(type==LOOKUPS)
- cp = nv_getv(np,handle);
- else
+ if(dp)
*dp = nv_getn(np,handle);
+ else
+ cp = nv_getv(np,handle);
}
if(bp== &block)
block_done(bp);
@@ -456,13 +456,13 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
static char* lookups(Namval_t *np, Namfun_t *handle)
{
- return(lookup(np,LOOKUPS,(Sfdouble_t*)0,handle));
+ return(lookup(np,(Sfdouble_t*)0,handle));
}
static Sfdouble_t lookupn(Namval_t *np, Namfun_t *handle)
{
Sfdouble_t d;
- lookup(np,LOOKUPN, &d ,handle);
+ lookup(np,&d,handle);
return(d);
}
@@ -563,10 +563,11 @@ char *nv_setdisc(register Namval_t* np,register const char *event,Namval_t *acti
else if(action)
{
Namdisc_t *dp = (Namdisc_t*)vp->fun.disc;
- if(type==LOOKUPS)
+ if(type==LOOKUP)
+ {
dp->getval = lookups;
- else if(type==LOOKUPN)
dp->getnum = lookupn;
+ }
vp->disc[type] = action;
}
else |
|
Sure, I will check it out. I was unable to |
|
The getn discipline was never removed after all, because doing so was causing regressions, IIRC (it's two years later and apparently I got sick of it and never posted). There is only one known corner-case crash now anyway, this one. It is still reproducible, so I decided to take another look with fresh eyes and two more years of experience. The problem seems to be that the global So, the fix should be simple: reset blist to NULL when the shell throws the error. This should be done in WIth that, there are no more known crashing scenarios with the Patch v1diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h
index 0792a0471..900ba02d8 100644
--- a/src/cmd/ksh93/include/shell.h
+++ b/src/cmd/ksh93/include/shell.h
@@ -389,6 +389,7 @@ struct Shell_s
/* nv_putsub() hack for nv_create() to avoid double arithmetic evaluation */
char nv_putsub_already_called_sh_arith;
int nv_putsub_idx; /* saves array index obtained by nv_putsub() using sh_arith() */
+ void *blist; /* 'struct blocked' list for nvdisc.c */
#if SHOPT_OPTIMIZE
char **argaddr; /* pointer to arguments for the loop invariants optimizer */
void *optlist; /* linked list of invariant nodes */
diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c
index 4c5b2cbae..1c32ee7fb 100644
--- a/src/cmd/ksh93/sh/fault.c
+++ b/src/cmd/ksh93/sh/fault.c
@@ -607,6 +607,7 @@ void sh_exit(int xno)
sh.mktype = 0;
sh.invoc_local = 0;
sh.tilde_block = 0;
+ sh.blist = NULL;
if(job.in_critical)
job_unlock();
if(pp->mode == SH_JMPSCRIPT && !pp->prev)
diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index d015b9c7b..be185b7f7 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -173,8 +173,6 @@ struct blocked
int isub;
};
-static struct blocked *blist;
-
#define isblocked(bp,type) ((bp)->flags & (1<<(type)))
#define block(bp,type) ((bp)->flags |= (1<<(type)))
#define unblock(bp,type) ((bp)->flags &= ~(1<<(type)))
@@ -189,7 +187,7 @@ static struct blocked *block_info(Namval_t *np, struct blocked *pp)
int isub=0;
if(nv_isarray(np) && (isub=nv_aindex(np)) < 0)
sub = nv_associative(np,NULL,NV_ACURRENT);
- for(bp=blist ; bp; bp=bp->next)
+ for(bp = sh.blist; bp; bp = bp->next)
{
if(bp->np==np && bp->sub==sub && bp->isub==isub)
return bp;
@@ -200,15 +198,15 @@ static struct blocked *block_info(Namval_t *np, struct blocked *pp)
pp->flags = 0;
pp->isub = isub;
pp->sub = sub;
- pp->next = blist;
- blist = pp;
+ pp->next = sh.blist;
+ sh.blist = pp;
}
return pp;
}
static void block_done(struct blocked *bp)
{
- blist = bp = bp->next;
+ sh.blist = bp = bp->next;
if(bp && (bp->isub>=0 || bp->sub))
nv_putsub(bp->np, bp->sub,(bp->isub<0?0:bp->isub)|ARRAY_SETSUB);
} |
|
Patch v1 works, but is actually a kludge. Further research turned up that the problem is that It's fine for nv_get{v,n} to longjmp, but not until after restoring state in lookup(). This patch does that, and I think it's a better way to fix this crash. Patch v2diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c
index d015b9c7b..f16ad9bba 100644
--- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -434,13 +434,6 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
}
if(nv_isarray(np))
np->nvalue.up = up;
- if(!cp)
- {
- if(type==LOOKUPS)
- cp = nv_getv(np,handle);
- else
- *dp = nv_getn(np,handle);
- }
if(bp== &block)
block_done(bp);
if(nq && nq->nvalue.rp && nq->nvalue.rp->running==1)
@@ -451,6 +444,14 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
if(jmpval >= SH_JMPFUN)
siglongjmp(*sh.jmplist,jmpval);
sh_sigcheck();
+ /* nv_get{v,n} may throw an error and longjmp, so must come after restoring state */
+ if(!cp)
+ {
+ if(type==LOOKUPS)
+ cp = nv_getv(np,handle);
+ else
+ *dp = nv_getn(np,handle);
+ }
return cp;
}
|
Reproducer: see the test added in tests/variables.sh. Analysis: The global 'struct blocked *blist' variable in nvdisc.c, which is the starting point for all the bp pointers, gets invalidated when the arithmetic subsystem throws an error in the reproducer. Which is obvious, because the struct blocked items in the list are all local/automatic variables in the assign() and lookup() functions, which no longer exist after a longjmp. The root problem is that an error is thrown during the nv_getn() call, so that block_done() is not getting called after the longjmp, so the global blist variable is not reset, causing a probable crash on any subsequent use of a discipline function. src/cmd/ksh93/sh/nvdisc.c: lookup(): - Delay calling nv_getv or nv_getn until after restoring all state, including after calling block_done(). src/cmd/ksh93/sh.1: - Finally add documentation for the getn discipline. Resolves: #435
I just discovered that, contrary to all the documentation, ksh understands not four initial kinds of discipline functions, but five. There is an undocumented
getndiscipline:ksh/src/cmd/ksh93/data/variables.c
Line 111 in 3f8add4
I've experimentally found that the
getndiscipline is likeget, but for arithmetic expressions. For instance, for$foo, the discipline functionfoo.getis called, but for$((foo))(with no dollar sign beforefoo), the discipline functionfoo.getnis called(with no fallback to(edit: yeah, that's wrong, see further below). This is not documented anywhere and I cannot even find a single mention on Google.foo.getiffoo.getnis not defined)A search in the ast-open-archive repo reveals that the
getndiscipline was quietly added in version 2009-08-21 93t+.Annoyingly, there is no corresponding distinction for the
setdiscipline. There is nosetndiscipline and thefoo.setdiscipline function is called for bothfoo=barand((foo=1337)).So, there are some things for the community to consider. Here are my views:I think it's a bug thatfoo.getis not called for$((foo)). It's contrary to all the documentation to date, which says thatfoo.getis called if thefoovariable is referenced. Referencing it in an arithmetic expression without a preceding dollar sign is still referencing it.I think it's wrong that$((foo))and$(($foo))call different disciplines. These expansions should be and are widely assumed to be equivalent. (See also 5da8eb3)So I think we should removeIt's not even used in any of the other AST code in ast-open-archive, regression tests included. If anyone can find a trace of prior use, that might change things.getnand make arithmetic expressions callgetinstead. I don't think backwards compatibility is a major concern here asgetnis completely undocumented and no one seems to have discovered it before me.On the other hand, if we decide to keep and document
getn, perhaps we should introduce a correspondingsetnand make ksh callfoo.setninstead offoo.setfor$((foo)). But that would be a greater compatibility concern as the current behaviour of thesetdiscipline is widely known and documented.Or we could change nothing and just document the current situation. It does not seem to be possible to introduce consistency without introducing some incompatibility or another.
I tend to put a high priority on consistency,
so all things considered, removing.getnand folding it intogetseems to be the lesser evil to meThoughts, opinions?
The text was updated successfully, but these errors were encountered: