37208: check for restricted parameter before changing from unset to set

Also, return NULL from setnparam() on failure to retrieve value (avoids
null-pointer dereference); add some expository comments

2 files changed, 27 insertions, 8 deletions

diff --git a/ChangeLog b/ChangeLog
index 6c40a08e6..8a0f9c075 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2015-11-23  Barton E. Schaefer  <schaefer@zsh.org>
 
+	* 37208: Src/params.c: check for restricted parameter before
+	changing from unset to set; return NULL from setnparam() on
+	failure to retrieve value (avoids null-pointer dereference);
+	add some expository comments
+
 	* 37208: Test/V10parameter.ztst: re-run the "typeset" tests with
 	the private module loaded
 
diff --git a/Src/params.c b/Src/params.c
index 602f69f33..4600284f0 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -880,6 +880,10 @@ createparam(char *name, int flags)
 		zerr("read-only variable: %s", name);
 		return NULL;
 	    }
+	    if ((oldpm->node.flags & PM_RESTRICTED) && isset(RESTRICTED)) {
+		zerr("%s: restricted", name);
+		return NULL;
+	    }
 	    if (!(oldpm->node.flags & PM_UNSET) || (oldpm->node.flags & PM_SPECIAL)) {
 		oldpm->node.flags &= ~PM_UNSET;
 		if ((oldpm->node.flags & PM_SPECIAL) && oldpm->ename) {
@@ -890,10 +894,6 @@ createparam(char *name, int flags)
 		}
 		return NULL;
 	    }
-	    if ((oldpm->node.flags & PM_RESTRICTED) && isset(RESTRICTED)) {
-		zerr("%s: restricted", name);
-		return NULL;
-	    }
 
 	    pm = oldpm;
 	    pm->base = pm->width = 0;
@@ -2777,6 +2777,7 @@ assignsparam(char *s, char *val, int flags)
     if (!v && !(v = getvalue(&vbuf, &t, 1))) {
 	unqueue_signals();
 	zsfree(val);
+	/* errflag |= ERRFLAG_ERROR; */
 	return NULL;
     }
     if (flags & ASSPM_WARN_CREATE)
@@ -2926,6 +2927,7 @@ assignaparam(char *s, char **val, int flags)
 	if (!(v = fetchvalue(&vbuf, &t, 1, SCANPM_ASSIGNING))) {
 	    unqueue_signals();
 	    freearray(val);
+	    /* errflag |= ERRFLAG_ERROR; */
 	    return NULL;
 	}
 
@@ -2988,6 +2990,7 @@ sethparam(char *s, char **val)
 	return NULL;
     queue_signals();
     if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING))) {
+	DPUTS(!v, "BUG: assigning to undeclared associative array");
 	createparam(t, PM_HASHED);
 	checkcreate = isset(WARNCREATEGLOBAL) && locallevel > 0;
     } else if (!(PM_TYPE(v->pm->node.flags) & PM_HASHED) &&
@@ -3000,6 +3003,7 @@ sethparam(char *s, char **val)
     if (!v)
 	if (!(v = fetchvalue(&vbuf, &t, 1, SCANPM_ASSIGNING))) {
 	    unqueue_signals();
+	    /* errflag |= ERRFLAG_ERROR; */
 	    return NULL;
 	}
     if (checkcreate)
@@ -3061,8 +3065,11 @@ setnparam(char *s, mnumber val)
 	} else if (val.type & MN_INTEGER) {
 	    pm->base = outputradix;
 	}
-	v = getvalue(&vbuf, &t, 1);
-	DPUTS(!v, "BUG: value not found for new parameter");
+	if (!(v = getvalue(&vbuf, &t, 1))) {
+	    DPUTS(!v, "BUG: value not found for new parameter");
+	    /* errflag |= ERRFLAG_ERROR; */
+	    return NULL;
+	}
 	if (!was_unset && isset(WARNCREATEGLOBAL) && locallevel > 0)
 	    check_warn_create(v->pm, "numeric");
     }
@@ -3219,6 +3226,10 @@ unsetparam_pm(Param pm, int altflag, int exp)
  *
  * This could usefully be made type-specific, but then we need
  * to be more careful when calling the unset method directly.
+ *
+ * The "exp"licit parameter should be nonzero for assignments and the
+ * unset command, and zero for implicit unset (e.g., end of scope).
+ * Currently this is used only by some modules.
  */
 
 /**/
@@ -5105,8 +5116,11 @@ freeparamnode(HashNode hn)
 {
     Param pm = (Param) hn;
  
-    /* Since the second flag to unsetfn isn't used, I don't *
-     * know what its value should be.                       */
+    /* The second argument of unsetfn() is used by modules to
+     * differentiate "exp"licit unset from implicit unset, as when
+     * a parameter is going out of scope.  It's not clear which
+     * of these applies here, but passing 1 has always worked.
+     */
     if (delunset)
 	pm->gsu.s->unsetfn(pm, 1);
     zsfree(pm->node.nam);