atheik
commented
2 years ago diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index f029b6c7..ad6560d5 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -636,6 +636,8 @@ static void put_seconds(register Namval_t* np,const char *val,int flags,Namfun_t
np->nvalue.dp = new_of(double,0);
}
nv_putv(np, val, flags, fp);
+ if(nv_isattr(np,NV_LDOUBLE)==NV_LDOUBLE)
+ *(double*)np->nvalue.ldp = *np->nvalue.ldp;
d = *np->nvalue.dp;
timeofday(&tp);
*np->nvalue.dp = dtime(&tp)-d;
@@ -678,7 +680,9 @@ static void put_rand(register Namval_t* np,const char *val,int flags,Namfun_t *f
_nv_unset(np,NV_RDONLY);
return;
}
- if(flags&NV_INTEGER)
+ if((flags&NV_LDOUBLE)==NV_LDOUBLE)
+ n = *(Sfdouble_t*)val;
+ else if((flags&NV_DOUBLE)==NV_DOUBLE)
n = *(double*)val;
else
n = sh_arith(val);
@@ -747,8 +751,8 @@ static void put_lineno(Namval_t* np,const char *val,int flags,Namfun_t *fp)
_nv_unset(np,NV_RDONLY);
return;
}
- if(flags&NV_INTEGER)
- n = (Sfdouble_t)(*(double*)val);
+ if((flags&NV_LDOUBLE)==NV_LDOUBLE)
+ n = *(Sfdouble_t*)val;
else
n = sh_arith(val);
sh.st.firstline += (int)(nget_lineno(np,fp) + 1 - n);
diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index e0bab084..d62770fb 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -361,6 +361,8 @@ static void nv_restore(struct subshell *sp)
nv_setsize(mp,nv_size(np));
if(!(flags&NV_MINIMAL))
mp->nvenv = np->nvenv;
+ if(mp->nvfun && !mp->nvfun->nofree)
+ free((char*)mp->nvfun);
mp->nvfun = np->nvfun;
if(np->nvfun && nofree)
np->nvfun->nofree = nofree;
@@ -371,7 +373,24 @@ static void nv_restore(struct subshell *sp)
}
mp->nvflag = np->nvflag|(flags&NV_MINIMAL);
if(nv_cover(mp))
- nv_putval(mp,nv_getval(np),NV_RDONLY);
+ {
+ char *val;
+ int newattrs = NV_NOFREE|np->nvflag;
+ if(nv_isattr(np,NV_INTEGER))
+ {
+ Sfdouble_t d = nv_getnum(np);
+ val = sh_memdup((char*)&d,sizeof(d));
+ newattrs |= NV_LDOUBLE;
+ }
+ else
+ val = nv_getval(np);
+ nv_putval(mp,val,NV_RDONLY|newattrs);
+ nv_offattr(mp,newattrs^nv_isattr(np,newattrs));
+ if(val && val!=Empty && val!=np->nvalue.cp && val!=mp->nvalue.cp)
+ free(val);
+ if(np->nvalue.cp && np->nvalue.cp!=Empty && np->nvalue.cp!=mp->nvalue.cp && !nv_isattr(np,NV_NOFREE))
+ free((char*)np->nvalue.cp);
+ }
else
mp->nvalue = np->nvalue;
if(nofree && np->nvfun && !np->nvfun->nofree)
diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh
index 05d8040b..dff5bc2a 100755
--- a/src/cmd/ksh93/tests/leaks.sh
+++ b/src/cmd/ksh93/tests/leaks.sh
@@ -412,7 +412,7 @@ DO
DONE
# ======
-TEST title='variable with discipline function in subshell' known=y url=https://github.com/ksh93/ksh/issues/404
+TEST title='variable with discipline function in subshell'
DO
(SECONDS=1; LANG=C)
DONE
The following observations are written from the perspective of nv_restore()
and the made changes, I would be grateful if someone would review it:
- The preceding _nv_unset() does not free allocations in `mp' if it has
NV_NOFREE set.
- When `flags' contains NV_NOFREE, nv_putval() ends up freeing the previous
value if `mp' has no NV_NOFREE set, setting the attributes of `mp' according
to the `flags' parameter, and *assigning* `mp->nvalue.cp' to `val'.
- When `flags' contains NV_NOFREE, this necessitates that if `mp' has NV_INTEGER
set, we can only pass a pointer to the dynamically allocated copy of the number
returned by nv_getnum(). If we pass a string representation of the number as
returned by nv_getval(), `val' is treated as a numeric pointer type in some
discipline functions such as put_seconds(), leading to an incorrect value.
Removing NV_INTEGER from `flags' would suggest that `val' should be treated as
a string, but put_seconds() is a discipline function that only deals with
NV_INTEGER types. In addition to that, the string representation of the number
as returned by nv_getval() is short lived and further nv_getval() calls may
modify it, which would also result in an incorrect value for `SECONDS'.
- A downside of `flags' containing NV_NOFREE is that nv_putval() does not cast
`val' to the pointer type determined by `flags&NV_INTEGER' before derefencing
and setting the `mp->nvalue' member as determined by `mp->nvflag&NV_INTEGER',
which would mean that `val' of type `Sfdouble_t*' is treated as `double*' in
put_seconds(), leading to an incorrect value, for example.
- Note that when LINENO and RANDOM are restored using put_lineno() and
put_rand(), respectively, they never have the NV_INTEGER set so `val' gets
evaluated through sh_arith() and no corruption occurs.
- The code that does the pointer type handling of NV_INTEGER types when `flags'
does not contain NV_NOFREE is at:
https://github.com/ksh93/ksh/blob/333a8ca/src/cmd/ksh93/sh/name.c#L1638-L1796
and is not run when `flags' contains NV_NOFREE because the nv_putval() that
is called from nv_putv(), which is called from the discipline function,
returns from the function at
https://github.com/ksh93/ksh/blob/333a8ca/src/cmd/ksh93/sh/name.c#L1615
. The least disruptive way to work around this is to modify the discipline
functions that deal with NV_INTEGER types such that they can do a proper cast
also when `(flags&NV_LDOUBLE)==NV_LDOUBLE', as that is the corresponding
attribute to the `Sfdouble_t' type used by the return value of nv_getnum().
Note that in put_seconds(), the nv_putv() call has already set the attributes
of `mp' according to `flags' so it is proper to check the attributes instead
of `flags'. The discipline functions of special variables that deal with
NV_INTEGER types do not check the exact type since the casts are written to
be synchronous with their attributes as defined at
https://github.com/ksh93/ksh/blob/333a8ca/src/cmd/ksh93/data/variables.c#L32-L109
. As a side note, the cast to `double*' is wrong in put_lineno() as LINENO
only has the NV_INTEGER attribute which means that the following is never true
`(flags&NV_DOUBLE)==NV_DOUBLE && (flags&NV_LDOUBLE)!=NV_LDOUBLE'. In the case
that `flags&NV_INTEGER' is true and there is no further type specifying
attribute, the value should be cast to `Sfdouble_t*'. `bin/shtests options'
has a scenario were `flags&NV_INTEGER' is true and you can see that `n' is
wrong after the assignment if you print it:
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index f029b6c7..0df5f51d 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -748,7 +748,12 @@ static void put_lineno(Namval_t* np,const char *val,int flags,Namfun_t *fp)
return;
}
if(flags&NV_INTEGER)
+ {
n = (Sfdouble_t)(*(double*)val);
+ sfprintf(sfstderr,"(wrong) n: %lf\n",(double)n); sfsync(sfstderr);
+ n = (Sfdouble_t)(*(Sfdouble_t*)val);
+ sfprintf(sfstderr,"(correct) n: %lf\n",(double)n); sfsync(sfstderr);
+ }
else
n = sh_arith(val);
sh.st.firstline += (int)(nget_lineno(np,fp) + 1 - n);
. I fail to see how the changes in commit 23b7a16 would have any effect.
- Some discipline functions of special variables such as put_lineno() do not set
`mp->nvalue.cp' when they end up being called from nv_putval(). This is due to
them not calling nv_putv() and allows for them to use sh_arith() and deal with
`val' being either string or an NV_INTEGER type. In this case we can free the
dynamically allocated copy of the number returned nv_getnum().
- We can free the value of `np', but again note that discipline functions of
special variables such as put_lang(), put_ifs(), and put_restricted() can set
`mp->nvalue.cp' to `np->nvalue.cp' meaning that then the value should not be
freed.
- Note that if `flags' does not contain NV_NOFREE, the *contents* of `val' are
copied to `mp->nvalue.cp'. In my opinion, freeing `mp' allocations without
passing NV_NOFREE to nv_putval() is convoluted and I have not managed to find
a way to do it without breaking something.
McDutchie
Regression test for a memory leak:
Unfortunately, there is at least one memory leak. But it is not introduced by Harald's fix -- it is the same before and after:
Originally posted by @McDutchie in https://github.com/ksh93/ksh/issues/253#issuecomment-815308466