Commit Diff

Commit:
95448b627ab3c0c205b0677309e4487484658a12
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
template: fix possible use-after-free and drop the `render' sugar if some of the tp_* function fails, the inner code could free the template too (that's what galileo does), so `tp->tp_ret' would access free'd memory! Instead, add a local variable `tp_ret' to keep track of the return code. While here, drop the `render' shortand that adds the argument call, the template_reset function and adapt the regress.
Actions:
Patch | Tree
commit - ee34715e9bf37e3f13a9d07b2ec1d817386c41fb
commit + 95448b627ab3c0c205b0677309e4487484658a12
blob - 9f7c33896bdb131bad0479e007b5b5ab8fcb46e1
blob + d38f3dcbcaf3d10e3d4da1ad55b2a53a5d46a785
--- fcgi.c
+++ fcgi.c
@@ -802,10 +802,8 @@ clt_tp_puts(struct template *tp, const char *str)
 {
 	struct client		*clt = tp->tp_arg;
 
-	if (clt_puts(clt, str) == -1) {
-		tp->tp_ret = -1;
+	if (clt_puts(clt, str) == -1)
 		return (-1);
-	}
 
 	return (0);
 }
@@ -815,10 +813,8 @@ clt_tp_putc(struct template *tp, int c)
 {
 	struct client		*clt = tp->tp_arg;
 
-	if (clt_putc(clt, c) == -1) {
-		tp->tp_ret = -1;
+	if (clt_putc(clt, c) == -1)
 		return (-1);
-	}
 
 	return (0);
 }
blob - 552e6b568fd81e12154d647cee396d3190cd4ed4
blob + f3e96dbd4607647561536b0725e2f933bc49400b
--- template/parse.y
+++ template/parse.y
@@ -136,9 +136,9 @@ verbatims	: /* empty */
 
 raw		: STRING {
 			dbg();
-			printf("if (tp->tp_puts(tp, ");
+			printf("if ((tp_ret = tp->tp_puts(tp, ");
 			printq($1);
-			printf(") == -1) goto err;\n");
+			printf(")) == -1) goto err;\n");
 
 			free($1);
 		}
@@ -146,12 +146,12 @@ raw		: STRING {
 
 block		: define body end {
 			printf("err:\n");
-			puts("return tp->tp_ret;");
+			puts("return tp_ret;");
 			puts("}");
 			in_define = 0;
 		}
 		| define body finally end {
-			puts("return tp->tp_ret;");
+			puts("return tp_ret;");
 			puts("}");
 			in_define = 0;
 		}
@@ -162,6 +162,7 @@ define		: '{' DEFINE string '}' {
 
 			dbg();
 			printf("int\n%s\n{\n", $3);
+			puts("int tp_ret = 0;");
 
 			free($3);
 		}
@@ -175,11 +176,7 @@ body		: /* empty */
 
 special		: '{' RENDER string '}' {
 			dbg();
-			if (strrchr($3, ')') != NULL)
-				printf("if (%s == -1) goto err;\n", $3);
-			else
-				printf("if (%s != NULL && %s(tp) == -1) "
-				    "goto err;\n", $3, $3);
+			printf("if ((tp_ret = %s) == -1) goto err;\n", $3);
 			free($3);
 		}
 		| printf
@@ -187,20 +184,23 @@ special		: '{' RENDER string '}' {
 		| loop
 		| '{' string '|' UNSAFE '}' {
 			dbg();
-			printf("if (tp->tp_puts(tp, %s) == -1) goto err;\n",
+			printf("if ((tp_ret = tp->tp_puts(tp, %s)) == -1)\n",
 			    $2);
+			puts("goto err;");
 			free($2);
 		}
 		| '{' string '|' URLESCAPE '}' {
 			dbg();
-			printf("if (tp_urlescape(tp, %s) == -1) goto err;\n",
+			printf("if ((tp_ret = tp_urlescape(tp, %s)) == -1)\n",
 			    $2);
+			puts("goto err;");
 			free($2);
 		}
 		| '{' string '}' {
 			dbg();
-			printf("if (tp->tp_escape(tp, %s) == -1) goto err;\n",
+			printf("if ((tp_ret = tp->tp_escape(tp, %s)) == -1)\n",
 			    $2);
+			puts("goto err;");
 			free($2);
 		}
 		;
@@ -210,7 +210,8 @@ printf		: '{' PRINTF {
 			printf("if (asprintf(&tp->tp_tmp, ");
 		} printfargs '}' {
 			printf(") == -1)\n goto err;\n");
-			puts("if (tp->tp_escape(tp, tp->tp_tmp) == -1)");
+			puts("if ((tp_ret = tp->tp_escape(tp, tp->tp_tmp)) "
+			    "== -1)");
 			puts("goto err;");
 			puts("free(tp->tp_tmp);");
 			puts("tp->tp_tmp = NULL;");
blob - 72fc2491d73a975d4a090dbe73551f9a3bd990ee
blob + f7aef5e2c673f80c7bfad865930f815b8c58859b
--- template/regress/03-block.tmpl
+++ template/regress/03-block.tmpl
@@ -9,7 +9,6 @@
 	</head>
 	<body>
 		<h1>{{ title }}</h1>
-		{{ render tp->tp_body }}
 	</body>
 </html>
 {{ finally }}
blob - 87bad8ee4581a664cf2097201d2d371369bcf9b1
blob + 4b378ef895124815cb89173a7604d0c8630d3bed
--- template/regress/04-flow.tmpl
+++ template/regress/04-flow.tmpl
@@ -22,7 +22,6 @@
 		{{ else }}
 			<p>"{{ title }}" doesn't have a '*' in it</p>
 		{{ end }}
-		{{ render tp->tp_body }}
 	</body>
 </html>
 {{ finally }}
blob - b2dcc4306bba2066a2b6162240a848836820a343
blob + 50bf6c0ec4dfcaec1fd63cba01c46d176957b614
--- template/regress/06-escape.tmpl
+++ template/regress/06-escape.tmpl
@@ -8,7 +8,6 @@
 	</head>
 	<body>
 		<h1>{{ title | unsafe }}</h1>
-		{{ render tp->tp_body }}
 	</body>
 </html>
 {{ end }}
blob - 079c1cdcd7646cc4367a33ddbc44e91dbe465c30
blob + 587f2ed68d2caf23a4828887385dae846d090cfe
--- template/regress/runbase.c
+++ template/regress/runbase.c
@@ -27,10 +27,8 @@ my_putc(struct template *tp, int c)
 {
 	FILE	*fp = tp->tp_arg;
 
-	if (putc(c, fp) < 0) {
-		tp->tp_ret = -1;
+	if (putc(c, fp) < 0)
 		return (-1);
-	}
 
 	return (0);
 }
@@ -40,10 +38,8 @@ my_puts(struct template *tp, const char *s)
 {
 	FILE	*fp = tp->tp_arg;
 
-	if (fputs(s, fp) < 0) {
-		tp->tp_ret = -1;
+	if (fputs(s, fp) < 0)
 		return (-1);
-	}
 
 	return (0);
 }
@@ -56,12 +52,12 @@ main(int argc, char **argv)
 	if ((tp = template(stdout, my_puts, my_putc)) == NULL)
 		err(1, "template");
 
-	base(tp, " *hello* ");
+	if (base(tp, " *hello* ") == -1)
+		return (1);
 	puts("");
 
-	template_reset(tp);
-
-	base(tp, "<hello>");
+	if (base(tp, "<hello>") == -1)
+		return (1);
 	puts("");
 
 	free(tp);
blob - 73afb09894b94e6728dbf1905ec8dae81608924f
blob + e5a35a7dfefa2010c52a71d596f2aa798602ab39
--- template/regress/runlist.c
+++ template/regress/runlist.c
@@ -28,10 +28,8 @@ my_putc(struct template *tp, int c)
 {
 	FILE	*fp = tp->tp_arg;
 
-	if (putc(c, fp) < 0) {
-		tp->tp_ret = -1;
+	if (putc(c, fp) < 0)
 		return (-1);
-	}
 
 	return (0);
 }
@@ -41,10 +39,8 @@ my_puts(struct template *tp, const char *s)
 {
 	FILE	*fp = tp->tp_arg;
 
-	if (fputs(s, fp) < 0) {
-		tp->tp_ret = -1;
+	if (fputs(s, fp) < 0)
 		return (-1);
-	}
 
 	return (0);
 }
@@ -69,7 +65,8 @@ main(int argc, char **argv)
 		TAILQ_INSERT_TAIL(&head, np, entries);
 	}
 
-	base(tp, &head);
+	if (base(tp, &head) == -1)
+		return (1);
 	puts("");
 
 	while ((np = TAILQ_FIRST(&head))) {
@@ -78,9 +75,8 @@ main(int argc, char **argv)
 		free(np);
 	}
 
-	template_reset(tp);
-
-	base(tp, &head);
+	if (base(tp, &head) == -1)
+		return (1);
 	puts("");
 
 	free(tp);
blob - c09d3b9a06921d1759263225b112bbeb5741f4b2
blob + 618f25c398ce5cfd534311076b139b2128826cf5
--- template/tmpl.c
+++ template/tmpl.c
@@ -31,10 +31,8 @@ tp_urlescape(struct template *tp, const char *str)
 		    isspace((unsigned char)*str) ||
 		    *str == '\'' || *str == '"' || *str == '\\') {
 			r = snprintf(tmp, sizeof(tmp), "%%%2X", *str);
-			if (r < 0  || (size_t)r >= sizeof(tmp)) {
-				tp->tp_ret = -1;
-				return (-1);
-			}
+			if (r < 0  || (size_t)r >= sizeof(tmp))
+				return (0);
 			if (tp->tp_puts(tp, tmp) == -1)
 				return (-1);
 		} else {
@@ -73,10 +71,8 @@ tp_htmlescape(struct template *tp, const char *str)
 			break;
 		}
 
-		if (r == -1) {
-			tp->tp_ret = -1;
+		if (r == -1)
 			return (-1);
-		}
 	}
 
 	return (0);
@@ -99,12 +95,6 @@ template(void *arg, tmpl_puts putsfn, tmpl_putc putcfn
 }
 
 void
-template_reset(struct template *tp)
-{
-	tp->tp_ret = 0;
-}
-
-void
 template_free(struct template *tp)
 {
 	free(tp->tp_tmp);
blob - 0de4f24796d823d76a6882bca883da23eb005c74
blob + a1ac0a10b029ac7e14728d1a670c61bb9cd195c1
--- template/tmpl.h
+++ template/tmpl.h
@@ -29,8 +29,6 @@ struct template {
 	tmpl_puts	 tp_puts;
 	tmpl_putc	 tp_putc;
 
-	int		 tp_ret;
-
 	int		(*tp_body)(struct template *);
 };
 
@@ -38,7 +36,6 @@ int		 tp_urlescape(struct template *, const char *);
 int		 tp_htmlescape(struct template *, const char *);
 
 struct template	*template(void *, tmpl_puts, tmpl_putc);
-void		 template_reset(struct template *);
 void		 template_free(struct template *);
 
 #endif