Re: httpd: don't send the fastcgi param struct over imsg and tweaks

On Fri Jul 17, 2026 at 11:57:47PM +0200, Kirill A. Korinsky wrote:
> On Fri, 17 Jul 2026 22:02:13 +0200,
> Rafael Sadowski <[email protected]> wrote:
> > 
> >
> 
> Mostly good, a few minor findings:
> 
> > -	if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, c) != 0) {
> > -		log_warn("%s: failed to compose IMSG_CFG_FCGI imsg for "
> > -		    "`%s'", __func__, srv_conf->name);
> > -		free(iov);
> > -		return (-1);
> > +		fpmsg.id = srv_conf->id;
> > +		fpmsg.namelen = strlen(fp->name);
> > +		fpmsg.vallen = strlen(fp->value);
> > +
> > +		iov[0].iov_base = &fpmsg;
> > +		iov[0].iov_len = sizeof(fpmsg);
> > +		iov[1].iov_base = fp->name;
> > +		iov[1].iov_len = fpmsg.namelen;
> > +		iov[2].iov_base = fp->value;
> > +		iov[2].iov_len = fpmsg.vallen;
> > +
> > +		if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, 3)
> > +		    != 0) {
> > +			log_warn("%s: failed to compose IMSG_CFG_FCGI "
> > +			    "for `%s'", __func__, srv_conf->name);
> > +			return (-1);
> > +		}
> 
> Here tricky part: if not first proc_composev() call fails, early messages
> are alread commited.
> 
> Before that refactoring here was none or all contract, now it none,
> something or all.

Yes and No. That's a very good point if you look at the diff in
isolation, but we've already sorted that out with the [1] diff:

+	if ((srv->srv_conf.flags & SRVFLAG_LOCATION) == 0) {
+		/* Configure TLS if necessary. */
+		if (config_setserver_tls(env, srv) != 0)
+			return (-1);
+	}
+
+	/* Configure FCGI parameters if necessary. */
+	if (config_setserver_fcgiparams(env, srv) != 0)
+		return (-1);
+

You've given your OK for this. Here, we check the return value and exit
with -1 if a proc_composev fails. Sure we could quit with fail()
directly but I think it's better to push the error up the chain. In the
end with end up with a fail().

1: https://marc.info/?l=openbsd-tech&m=178429421655440&w=2

> 
> > @@ -813,29 +814,33 @@ fcgiflags	: SOCKET STRING {
> >  			if ((param = calloc(1, sizeof(*param))) == NULL)
> >  				fatal("out of memory");
> >  
> > -			if (strlcpy(param->name, $2, sizeof(param->name)) >=
> > -			    sizeof(param->name)) {
> > -				yyerror("fastcgi_param name truncated");
> > +			if (strlen($2) > HTTPD_FCGI_NAME_MAX - 1) {
> > +				yyerror("fastcgi param name too long (max %d)",
> > +				    HTTPD_FCGI_NAME_MAX - 1);
> >  				free($2);
> >  				free($3);
> > -				free(param);
> >  				YYERROR;
> >  			}
> > -			if (strlcpy(param->value, $3, sizeof(param->value)) >=
> > -			    sizeof(param->value)) {
> > -				yyerror("fastcgi_param value truncated");
> > +
> > +			if (strlen($3) > HTTPD_FCGI_VAL_MAX - 1) {
> > +				yyerror("fastcgi param value too long (max %d)",
> > +				    HTTPD_FCGI_VAL_MAX - 1);
> >  				free($2);
> >  				free($3);
> > -				free(param);
> >  				YYERROR;
> >  			}
> > +
> > +			if ((param->name = strdup($2)) == NULL ||
> > +			    (param->value = strdup($3)) == NULL)
> > +				fatal("out of memory");
> > +
> >  			free($2);
> >  			free($3);
> >  
> >  			DPRINTF("[%s,%s,%d]: adding param \"%s\" value \"%s\"",
> >  			    srv_conf->location, srv_conf->name, srv_conf->id,
> >  			    param->name, param->value);
> > -			TAILQ_INSERT_HEAD(&srv_conf->fcgiparams, param, entry);
> > +			TAILQ_INSERT_TAIL(&srv_conf->fcgiparams, param, entry);
> >  		}
> >  		| STRIP NUMBER			{
> >  			if ($2 < 0 || $2 > INT_MAX) {
> 
> I think here you're leaking param on not sucessfull exit.

Yes I fix it by placing the calloc after the _MAX checks.



diff --git a/config.c b/config.c
index 146ed2c..ec1ec26 100644
--- a/config.c
+++ b/config.c
@@ -348,67 +348,52 @@ config_settls(struct httpd *env, struct server *srv, enum tls_config_type type,
 int
 config_getserver_fcgiparams(struct httpd *env, struct imsg *imsg)
 {
-	struct server		*srv;
-	struct server_config	*srv_conf, *iconf;
-	struct fastcgi_param	*fp;
-	uint32_t		 id;
-	size_t			 c, nc, len;
-	uint8_t			*p = imsg->data;
-
-	len = sizeof(nc) + sizeof(id);
-	if (IMSG_DATA_SIZE(imsg) < len) {
-		log_debug("%s: invalid message length", __func__);
+	struct server_config		*srv_conf;
+	struct fastcgi_param		*fp;
+	struct fastcgi_param_imsg	 fpmsg;
+	struct ibuf			 ibuf;
+
+	if (imsg_get_ibuf(imsg, &ibuf) == -1 ||
+	    ibuf_get(&ibuf, &fpmsg, sizeof(fpmsg)) == -1) {
+		log_debug("%s: invalid message", __func__);
 		return (-1);
 	}
 
-	memcpy(&nc, p, sizeof(nc));	/* number of params */
-	p += sizeof(nc);
-
-	memcpy(&id, p, sizeof(id));	/* server conf id */
-	srv_conf = serverconfig_byid(id);
-	p += sizeof(id);
-
-	len += nc*sizeof(*fp);
-	if (IMSG_DATA_SIZE(imsg) < len) {
-		log_debug("%s: invalid message length", __func__);
+	if ((srv_conf = serverconfig_byid(fpmsg.id)) == NULL) {
+		log_debug("%s: invalid config id", __func__);
 		return (-1);
 	}
 
-	/* Find associated server config */
-	TAILQ_FOREACH(srv, env->sc_servers, srv_entry) {
-		if (srv->srv_conf.id == id) {
-			srv_conf = &srv->srv_conf;
-			break;
-		}
-		TAILQ_FOREACH(iconf, &srv->srv_hosts, entry) {
-			if (iconf->id == id) {
-				srv_conf = iconf;
-				break;
-			}
-		}
+	if (fpmsg.namelen > HTTPD_FCGI_NAME_MAX - 1 ||
+	    fpmsg.vallen > HTTPD_FCGI_VAL_MAX - 1) {
+		log_debug("%s: fastcgi_param too long", __func__);
+		return (-1);
 	}
 
-	/* Fetch FCGI parameters */
-	for (c = 0; c < nc; c++) {
-		if ((fp = calloc(1, sizeof(*fp))) == NULL)
-			fatalx("fcgiparams out of memory");
-		memcpy(fp, p, sizeof(*fp));
-		TAILQ_INSERT_HEAD(&srv_conf->fcgiparams, fp, entry);
+	if ((fp = calloc(1, sizeof(*fp))) == NULL)
+		fatal("fastcgi_param out of memory");
 
-		p += sizeof(*fp);
+	fp->name = ibuf_get_string(&ibuf, fpmsg.namelen);
+	fp->value = ibuf_get_string(&ibuf, fpmsg.vallen);
+	if (fp->name == NULL || fp->value == NULL) {
+		free(fp->name);
+		free(fp->value);
+		free(fp);
+		return (-1);
 	}
 
+	TAILQ_INSERT_TAIL(&srv_conf->fcgiparams, fp, entry);
 	return (0);
 }
 
 int
 config_setserver_fcgiparams(struct httpd *env, struct server *srv)
 {
-	struct privsep		*ps = env->sc_ps;
-	struct server_config	*srv_conf = &srv->srv_conf;
-	struct fastcgi_param	 *fp;
-	struct iovec		 *iov;
-	size_t			 c = 0, nc = 0;
+	struct privsep			*ps = env->sc_ps;
+	struct server_config		*srv_conf = &srv->srv_conf;
+	struct fastcgi_param		*fp;
+	struct fastcgi_param_imsg	fpmsg;
+	struct iovec			iov[3];
 
 	DPRINTF("%s: sending fcgiparam for \"%s[%u]\" to %s fd %d", __func__,
 	    srv_conf->name, srv_conf->id, ps->ps_title[PROC_SERVER],
@@ -418,28 +403,24 @@ config_setserver_fcgiparams(struct httpd *env, struct server *srv)
 		return (0);
 
 	TAILQ_FOREACH(fp, &srv_conf->fcgiparams, entry) {
-		nc++;
-	}
-	if ((iov = calloc(nc + 2, sizeof(*iov))) == NULL)
-		return (-1);
-
-	iov[c].iov_base = &nc;			/* number of params */
-	iov[c++].iov_len = sizeof(nc);
-	iov[c].iov_base = &srv_conf->id;	/* server config id */
-	iov[c++].iov_len = sizeof(srv_conf->id);
-
-	TAILQ_FOREACH(fp, &srv_conf->fcgiparams, entry) {	/* push FCGI params */
-		iov[c].iov_base = fp;
-		iov[c++].iov_len = sizeof(*fp);
-	}
-	if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, c) != 0) {
-		log_warn("%s: failed to compose IMSG_CFG_FCGI imsg for "
-		    "`%s'", __func__, srv_conf->name);
-		free(iov);
-		return (-1);
+		fpmsg.id = srv_conf->id;
+		fpmsg.namelen = strlen(fp->name);
+		fpmsg.vallen = strlen(fp->value);
+
+		iov[0].iov_base = &fpmsg;
+		iov[0].iov_len = sizeof(fpmsg);
+		iov[1].iov_base = fp->name;
+		iov[1].iov_len = fpmsg.namelen;
+		iov[2].iov_base = fp->value;
+		iov[2].iov_len = fpmsg.vallen;
+
+		if (proc_composev(ps, PROC_SERVER, IMSG_CFG_FCGI, iov, 3)
+		    != 0) {
+			log_warn("%s: failed to compose IMSG_CFG_FCGI "
+			    "for `%s'", __func__, srv_conf->name);
+			return (-1);
+		}
 	}
-	free(iov);
-
 	return (0);
 }
 
@@ -727,6 +708,8 @@ config_getserver(struct httpd *env, struct imsg *imsg)
 	memcpy(&srv->srv_conf, &srv_conf, sizeof(srv->srv_conf));
 	srv->srv_s = fd;
 
+	TAILQ_INIT(&srv->srv_conf.fcgiparams);
+
 	if (config_getserver_auth(env, &srv->srv_conf) != 0)
 		goto fail;
 
diff --git a/httpd.h b/httpd.h
index 740721e..3cce5d1 100644
--- a/httpd.h
+++ b/httpd.h
@@ -65,8 +65,8 @@
 #define HTTPD_TLS_CIPHERS	"secure"
 #define HTTPD_TLS_DHE_PARAMS	"none"
 #define HTTPD_TLS_ECDHE_CURVES	"default"
-#define HTTPD_FCGI_NAME_MAX	511
-#define HTTPD_FCGI_VAL_MAX	511
+#define HTTPD_FCGI_NAME_MAX	512
+#define HTTPD_FCGI_VAL_MAX	8192
 #define FD_RESERVE		5
 
 #define SERVER_MAX_CLIENTS	1024
@@ -440,9 +440,15 @@ struct server_tls_ticket {
 	unsigned char	tt_key[TLS_TICKET_KEY_SIZE];
 };
 
+struct fastcgi_param_imsg {
+	uint32_t	id;	/* server conf id */
+	uint16_t	namelen;
+	uint16_t	vallen;
+};
+
 struct fastcgi_param {
-	char			name[HTTPD_FCGI_NAME_MAX];
-	char			value[HTTPD_FCGI_VAL_MAX];
+	char			*name;
+	char			*value;
 
 	TAILQ_ENTRY(fastcgi_param) entry;
 };
diff --git a/parse.y b/parse.y
index 65664e1..6640d0b 100644
--- a/parse.y
+++ b/parse.y
@@ -657,6 +657,7 @@ serveroptsl	: LISTEN ON STRING opttls port	{
 			srv = s;
 			srv_conf = &srv->srv_conf;
 			SPLAY_INIT(&srv->srv_clients);
+			TAILQ_INIT(&srv_conf->fcgiparams);
 		} '{' optnl serveropts_l '}'	{
 			struct server	*s = NULL;
 			uint64_t	 f;
@@ -810,32 +811,37 @@ fcgiflags	: SOCKET STRING {
 		| PARAM STRING STRING	{
 			struct fastcgi_param	*param;
 
-			if ((param = calloc(1, sizeof(*param))) == NULL)
-				fatal("out of memory");
-
-			if (strlcpy(param->name, $2, sizeof(param->name)) >=
-			    sizeof(param->name)) {
-				yyerror("fastcgi_param name truncated");
+			if (strlen($2) > HTTPD_FCGI_NAME_MAX - 1) {
+				yyerror("fastcgi param name too long (max %d)",
+				    HTTPD_FCGI_NAME_MAX - 1);
 				free($2);
 				free($3);
-				free(param);
 				YYERROR;
 			}
-			if (strlcpy(param->value, $3, sizeof(param->value)) >=
-			    sizeof(param->value)) {
-				yyerror("fastcgi_param value truncated");
+
+			if (strlen($3) > HTTPD_FCGI_VAL_MAX - 1) {
+				yyerror("fastcgi param value too long (max %d)",
+				    HTTPD_FCGI_VAL_MAX - 1);
 				free($2);
 				free($3);
-				free(param);
 				YYERROR;
 			}
+
+			if ((param = calloc(1, sizeof(*param))) == NULL)
+				fatal("out of memory");
+
+			if ((param->name = strdup($2)) == NULL ||
+			    (param->value = strdup($3)) == NULL)
+				fatal("out of memory");
+
 			free($2);
 			free($3);
 
 			DPRINTF("[%s,%s,%d]: adding param \"%s\" value \"%s\"",
 			    srv_conf->location, srv_conf->name, srv_conf->id,
 			    param->name, param->value);
-			TAILQ_INSERT_HEAD(&srv_conf->fcgiparams, param, entry);
+
+			TAILQ_INSERT_TAIL(&srv_conf->fcgiparams, param, entry);
 		}
 		| STRIP NUMBER			{
 			if ($2 < 0 || $2 > INT_MAX) {
diff --git a/server.c b/server.c
index 0731730..a12d6f9 100644
--- a/server.c
+++ b/server.c
@@ -483,8 +483,11 @@ serverconfig_free(struct server_config *srv_conf)
 	freezero(srv_conf->tls_cert, srv_conf->tls_cert_len);
 	freezero(srv_conf->tls_key, srv_conf->tls_key_len);
 
-	TAILQ_FOREACH_SAFE(param, &srv_conf->fcgiparams, entry, tparam)
+	TAILQ_FOREACH_SAFE(param, &srv_conf->fcgiparams, entry, tparam) {
+		free(param->name);
+		free(param->value);
 		free(param);
+	}
 }
 
 void