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

On Thu, 16 Jul 2026 11:30:09 +0200,
Rafael Sadowski <[email protected]> wrote:
> 
> Based on the "custom header" idea and found issues.
> 
> Send one imsg per param with a small fixed header (fastcgi_param_imsg)
> followed by the name and value bytes, and read it back with imsg_get_ibuf()
> and ibuf_get_string(). The struct now uses dynamic buffer instead of fixed,
> and the value limit is raised to 8192. Parse order is now preserved end-to-end
> instead of being reversed twice. This follows the config order:
> 
> ...
> fcgi_add_param: HTTP_USER_AGENT[15] => curl/8.21.0[11], total_len: 189
> fcgi_add_param: FCGI_TEST_A[11] => A[1], total_len: 217
> fcgi_add_param: FCGI_TEST_B[11] => B[1], total_len: 231
> fcgi_add_param: FCGI_TEST_C[11] => C[1], total_len: 245
> fcgi_add_param: FCGI_TEST_D[11] => D[1], total_len: 259
> fcgi_add_param: FCGI_TEST_E[11] => E[1], total_len: 273
> fcgi_add_param: FCGI_TEST_BB[12] => BB[2], total_len: 287
> ...
> 
> This diff not include the change in "httpd: do NOT send location
> fcgiparams ...": https://marc.info/?l=openbsd-tech&m=178397190636339&w=2
> 
> Feedback, OK?
> 
> Rafael
> 
> 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..257ecb7 100644
> --- a/parse.y
> +++ b/parse.y
> @@ -813,29 +813,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);

Are you sure that here no possible null case which will lead to crash?

Probably some tricky things like some fastcgi { param ... } inside a
location may attempt to write to null.

>  		}
>  		| 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
> 

-- 
wbr, Kirill