Fix a very long-standing error in handling .SHELL targets: this target

uses the brk_string function to parse the line. That function uses static
storage for both the expanded string and the returned argv[] vector.
The JobParseShell function simply stored away pointers into this static
storage. On the next use of something like ${FOO:O} this storage would
get overwritten with fatal results.

This also allows us to make the shells[] array const bringing us one step
further in making make WARNS=4 ready.

1 files changed, 125 insertions, 46 deletions

diff --git a/usr.bin/make/job.c b/usr.bin/make/job.c
index 0b86bb0..50571e4 100644
--- a/usr.bin/make/job.c
+++ b/usr.bin/make/job.c
@@ -206,14 +206,11 @@ static Shell    shells[] = {
     "v", "e",
 },
 };
-static Shell 	*commandShell = &shells[DEFSHELL];/* this is the shell to
-						   * which we pass all
-						   * commands in the Makefile.
-						   * It is set by the
-						   * Job_ParseShell function */
-char   		*shellPath = NULL,		  /* full pathname of
-						   * executable image */
-               	*shellName = NULL;		  /* last component of shell */
+static Shell 	*commandShell = NULL;	/* this is the shell to which we pass
+					 * all commands in the Makefile. It is
+					 * set by the Job_ParseShell function */
+char   		*shellPath = NULL,	/* full pathname of executable image */
+               	*shellName = NULL;	/* last component of shell */
 
 
 static int  	maxJobs;    	/* The most children we can run at once */
@@ -2071,9 +2068,90 @@ Job_Make(GNode *gn)
     (void) JobStart(gn, 0, NULL);
 }
 
+/*
+ * JobCopyShell:
+ *
+ * Make a new copy of the shell structure including a copy of the strings
+ * in it. This also defaults some fields in case they are NULL.
+ *
+ * The function returns a pointer to the new shell structure otherwise.
+ */
+static Shell *
+JobCopyShell(const Shell *osh)
+{
+	Shell *nsh;
+
+	nsh = emalloc(sizeof(*nsh));
+	nsh->name = estrdup(osh->name);
+
+	if (osh->echoOff != NULL)
+		nsh->echoOff = estrdup(osh->echoOff);
+	else
+		nsh->echoOff = NULL;
+	if (osh->echoOn != NULL)
+		nsh->echoOn = estrdup(osh->echoOn);
+	else
+		nsh->echoOn = NULL;
+	nsh->hasEchoCtl = osh->hasEchoCtl;
+
+	if (osh->noPrint != NULL)
+		nsh->noPrint = estrdup(osh->noPrint);
+	else
+		nsh->noPrint = NULL;
+	nsh->noPLen = osh->noPLen;
+
+	nsh->hasErrCtl = osh->hasErrCtl;
+	if (osh->errCheck == NULL)
+		nsh->errCheck = estrdup("");
+	else
+		nsh->errCheck = estrdup(osh->errCheck);
+	if (osh->ignErr == NULL)
+		nsh->ignErr = estrdup("%s");
+	else
+		nsh->ignErr = estrdup(osh->ignErr);
+
+	if (osh->echo == NULL)
+		nsh->echo = estrdup("");
+	else
+		nsh->echo = estrdup(osh->echo);
+
+	if (osh->exit == NULL)
+		nsh->exit = estrdup("");
+	else
+		nsh->exit = estrdup(osh->exit);
+
+	return (nsh);
+}
+
+/*
+ * JobFreeShell:
+ *
+ * Free a shell structure and all associated strings.
+ */
+static void
+JobFreeShell(Shell *sh)
+{
+
+	if (sh != NULL) {
+		free(sh->name);
+		free(sh->echoOff);
+		free(sh->echoOn);
+		free(sh->noPrint);
+		free(sh->errCheck);
+		free(sh->ignErr);
+		free(sh->echo);
+		free(sh->exit);
+		free(sh);
+	}
+}
+
 void
 Shell_Init(void)
 {
+
+    if (commandShell == NULL)
+	commandShell = JobCopyShell(&shells[DEFSHELL]);
+
     if (shellPath == NULL) {
 	/*
 	 * The user didn't specify a shell to use, so we are using the
@@ -2451,6 +2529,18 @@ Job_ParseShell(char *line)
 	     }
     }
 
+    /*
+     * Some checks (could be more)
+     */
+    if (fullSpec) {
+	if ((newShell.echoOn != NULL) ^ (newShell.echoOff != NULL))
+	    Parse_Error(PARSE_FATAL, "Shell must have either both echoOff and "
+		"echoOn or none of them");
+
+	if (newShell.echoOn != NULL && newShell.echoOff)
+	    newShell.hasEchoCtl = TRUE;
+    }
+
     if (path == NULL) {
 	/*
 	 * If no path was given, the user wants one of the pre-defined shells,
@@ -2460,16 +2550,13 @@ Job_ParseShell(char *line)
 	 */
 	if (newShell.name == NULL) {
 	    Parse_Error(PARSE_FATAL, "Neither path nor name specified");
-	    return(FAILURE);
-	} else {
-	    if ((sh = JobMatchShell(newShell.name)) == NULL) {
-		Parse_Error(PARSE_FATAL, "%s: no matching shell",
-			    newShell.name);
-		return(FAILURE);
-	    }
-	    commandShell = sh;
-	    shellName = newShell.name;
+	    return (FAILURE);
+	}
+	if ((sh = JobMatchShell(newShell.name)) == NULL) {
+	    Parse_Error(PARSE_FATAL, "%s: no matching shell", newShell.name);
+	    return (FAILURE);
 	}
+
     } else {
 	/*
 	 * The user provided a path. If s/he gave nothing else (fullSpec is
@@ -2478,45 +2565,37 @@ Job_ParseShell(char *line)
 	 * to a new location. In either case, we need to record the
 	 * path the user gave for the shell.
 	 */
-	shellPath = path;
-	path = strrchr(path, '/');
-	if (path == NULL) {
-	    path = shellPath;
-	} else {
-	    path += 1;
-	}
-	if (newShell.name != NULL) {
-	    shellName = newShell.name;
-	} else {
-	    shellName = path;
+	free(shellPath);
+	shellPath = estrdup(path);
+	if (newShell.name == NULL) {
+	    /* get the base name as the name */
+	    path = strrchr(path, '/');
+	    if (path == NULL) {
+	        path = shellPath;
+	    } else {
+	        path += 1;
+	    }
+	    newShell.name = path;
 	}
+
 	if (!fullSpec) {
-	    if ((sh = JobMatchShell(shellName)) == NULL) {
+	    if ((sh = JobMatchShell(newShell.name)) == NULL) {
 		Parse_Error(PARSE_FATAL, "%s: no matching shell",
-			    shellName);
-		return(FAILURE);
+		    newShell.name);
+		return (FAILURE);
 	    }
-	    commandShell = sh;
 	} else {
-	    commandShell = (Shell *) emalloc(sizeof(Shell));
-	    *commandShell = newShell;
+	    sh = &newShell;
 	}
     }
 
-    if (commandShell->echoOn && commandShell->echoOff) {
-	commandShell->hasEchoCtl = TRUE;
-    }
+    /* set the new shell */
+    JobFreeShell(commandShell);
+    commandShell = JobCopyShell(sh);
 
-    if (!commandShell->hasErrCtl) {
-	if (commandShell->errCheck == NULL) {
-	    commandShell->errCheck = "";
-	}
-	if (commandShell->ignErr == NULL) {
-	    commandShell->ignErr = "%s\n";
-	}
-    }
+    shellName = commandShell->name;
 
-    return SUCCESS;
+    return (SUCCESS);
 }
 
 /*-