Fix a crash when running 'unalias r'

[JohnoKing]

After commit ddaa145, the following set of commands now causes ksh to crash:

$ unalias history; unalias r
Memory fault

When ksh is compiled with -D_std_malloc, the crash always occurs when the r alias is removed with unalias r, although with vmalloc unalias history must be run first for the crash to occur. The crash message is also different when the native malloc is used instead of vmalloc:

$ unalias r
free(): invalid pointer
Abort

This crash happens because the NV_NOFREE flag disappears after _nv_unset, which results in nv_isattr becoming a no-op (as it always returns zero). nv_delete then attempts an invalid free, which causes the crash:

ksh/src/cmd/ksh93/bltins/typeset.c

Lines 1295 to 1296 in 05683ec

	if(!nv_isnull(np) || nv_size(np) || nv_isattr(np,~(NV_MINIMAL|NV_NOFREE)))
	_nv_unset(np,0);

ksh/src/cmd/ksh93/bltins/typeset.c

Line 1314 in 05683ec

nv_delete(np,troot,nv_isattr(np,NV_NOFREE));

The history and r aliases shouldn't be freed from memory by nv_delete because those aliases are given the NV_NOFREE flag:

ksh/src/cmd/ksh93/data/aliases.c

Lines 32 to 33 in 05683ec

	"history", NV_NOFREE, "hist -l",
	"r", NV_NOFREE, "hist -s",

This pull request fixes the crash by saving the NV_NOFREE flag for aliases to prevent an invalid free from occurring.

[McDutchie]

Looks like I done goofed again. Thank you for this fix!

[McDutchie]

I'm still going to give this some more thought before applying it, though. Maybe the real bug is that _nv_unset() discards a node's NV_NOFREE flag. Why on earth would it do such a thing? NV_NOFREE signifies that the node is in read-only memory, so that flag should never be unset.

I'm going to be busy with other things in the next few days, so this will have to wait for a while. Meanwhile if you have any thoughts on that I'd appreciate them.

[McDutchie]

Well, this diff

diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
index 2a0c216..d5d5276 100644
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -2536,7 +2536,7 @@ done:
 				env_delete(shp->env,nv_name(np));
 			if(!(flags&NV_EXPORT) ||  nv_isattr(np,NV_EXPORT))
 				np->nvenv = 0;
-			nv_setattr(np,0);
+			nv_setattr(np,nv_isattr(np,NV_NOFREE));
 		}
 		else
 		{

fixes the crash, but also causes multiple regressions:

	arrays.sh[575]: typeset foo[7] should not have one element
	attributes.sh[447]: typeset -pa does not list only index arrays
	cubetype.sh[193]: ${#cc[@]} != 2
	cubetype.sh[209]: ${#cc[@]} failed -- expected '3', got '4'
	cubetype.sh[153]: cc[0].x !=8
	cubetype.sh[163]: cc[2].len != cc[0].len
	cubetype.sh[165]: cc[2].count != 6
	cubetype.sh[173]: ${#cc[@]} != 2
	cubetype.sh[193]: ${#cc[@]} != 2
	cubetype.sh[209]: ${#cc[@]} failed -- expected '3', got '4'
	types.sh[358]: compound variable with array of types with no elements not working

So, I'll merge your fix then.

[McDutchie]

Actually, I think your fix can be made more concise. As of ec88886, aliases don't have virtual subshell tables. So there's no need to mark an alias node as unset first with _nv_unset(); we can simply nv_delete() it directly.

I've pushed a commit with that change to your fork. Since this is your pull request, please test it and let me know if you give it the OK.

[McDutchie]

Actually, never mind. My version introduces a memory leak. Apparently, we must _nv_unset() before calling nv_delete(). Which makes sense because nv_delete() only frees the node itself, _nv_unset() is what frees the value.

[McDutchie]

You're definitely not correct about one thing, though: nv_isattr is not a boolean value, it returns a value with the bit corresponding to the flag if that flag is set. The macro is defined as follows:

ksh/src/cmd/ksh93/include/nval.h

Line 205 in 7e5fd3e

#define nv_isattr(np,f) ((np)->nvflag & (f))

Note it uses a bitwise AND (&) and not a logical AND (&&). So you could even use this to test for multiple flags at the same time, as in nv_isattr(np,FLAG1|FLAG2|FLAG3) and it will return the flags that are set.