From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: memleak in (and small refactoring of) fcgi_parse_record
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org, Tracey Emery <tracey@traceyemery.net>
Date: Wed, 31 Aug 2022 18:04:13 +0200

On 2022/08/31 17:58:08 +0200, Omar Polo <op@omarpolo.com> wrote:
> gotwebd parses the fastcgi params into a list.  (I think this is a
> leftover from slowcgi, where that list is then used to fill the
> environment of the CGI process.)  However, the list is never free'd
> and also never looked at outside of fcgi_parse_params, so I think we
> can drop it.
> 
> I ended up looking at the fastcgi code for a different reason.  I've
> seen a crash in a bcopy in fcgi_parse_params once, and after
> rebuilding gotwebd with -O2 it never picked up SCRIPT_NAME
> correctly...  Turns out we're reading from an un-initialized variable
> `dr_buf' and got lucky since.

Ah, i double checked.  This was introduced by me in the previous
commit when switching from DOCUMENT_ROOT to SCRIPT_NAME.  Before
dr_buf was initialized before being read with

	dr_buf = &buf[1];

...

> While here I've slightly tweaked the code, in particular I've turned a
> 
> 	if (n > 0) {
> 		... code ...
> 	} else
> 		return
> 
> into a IMHO more clear
> 
> 	if (n == 0)
> 		return;
> 	...code...
> 
> and I'm making the matching of the variables stronger by ensuring that
> name_len is as long as the params we're interested in.
> 
> OK?
> 
> diff /home/op/w/got
> commit - 794662a4547b17c0243addf37d330d39e0eb5662
> path + /home/op/w/got
> blob - 865c86621023df5c6c3d96bfad927b4eab09a613
> file + gotwebd/fcgi.c
> --- gotwebd/fcgi.c
> +++ gotwebd/fcgi.c
> @@ -173,18 +173,14 @@ fcgi_parse_begin_request(uint8_t *buf, uint16_t n,
>  	}
>  
>  	c->request_started = 1;
> -
>  	c->id = id;
> -	SLIST_INIT(&c->env);
> -	c->env_count = 0;
>  }
>  
>  void
>  fcgi_parse_params(uint8_t *buf, uint16_t n, struct request *c, uint16_t id)
>  {
> -	struct env_val *env_entry;
>  	uint32_t name_len, val_len;
> -	uint8_t *sd, *dr_buf;
> +	uint8_t *sd, *val;
>  
>  	if (!c->request_started) {
>  		log_warn("FCGI_PARAMS without FCGI_BEGIN_REQUEST, ignoring");
> @@ -216,83 +212,65 @@ fcgi_parse_params(uint8_t *buf, uint16_t n, struct req
>  				return;
>  		}
>  
> -		if (n > 0) {
> -			if (buf[0] >> 7 == 0) {
> -				val_len = buf[0];
> -				n--;
> -				buf++;
> -			} else {
> -				if (n > 3) {
> -					val_len = ((buf[0] & 0x7f) << 24) +
> -					    (buf[1] << 16) + (buf[2] << 8) +
> -					     buf[3];
> -					n -= 4;
> -					buf += 4;
> -				} else
> -					return;
> -			}
> -		} else
> +		if (n == 0)
>  			return;
>  
> +		if (buf[0] >> 7 == 0) {
> +			val_len = buf[0];
> +			n--;
> +			buf++;
> +		} else {
> +			if (n > 3) {
> +				val_len = ((buf[0] & 0x7f) << 24) +
> +					(buf[1] << 16) + (buf[2] << 8) +
> +					buf[3];
> +				n -= 4;
> +				buf += 4;
> +			} else
> +				return;
> +		}
> +
>  		if (n < name_len + val_len)
>  			return;
>  
> -		if ((env_entry = malloc(sizeof(struct env_val))) == NULL) {
> -			log_warn("cannot malloc env_entry");
> -			return;
> -		}
> +		val = buf + name_len;
>  
> -		if ((env_entry->val = calloc(sizeof(char), name_len + val_len +
> -		    2)) == NULL) {
> -			log_warn("cannot allocate env_entry->val");
> -			free(env_entry);
> -			return;
> -		}
> +		if (c->querystring[0] == '\0' &&
> +		    val_len < MAX_QUERYSTRING &&
> +		    name_len == 12 &&
> +		    strncmp(buf, "QUERY_STRING", 12) == 0)
> +			strncpy(c->querystring, val, val_len);
>  
> -		bcopy(buf, env_entry->val, name_len);
> -		buf += name_len;
> -		n -= name_len;
> +		if (c->http_host[0] == '\0' &&
> +		    val_len < GOTWEBD_MAXTEXT &&
> +		    name_len == 9 &&
> +		    strncmp(buf, "HTTP_HOST", 9) == 0) {
> +			strncpy(c->http_host, val, val_len);
>  
> -		env_entry->val[name_len] = '\0';
> -		if (val_len < MAX_QUERYSTRING && strcmp(env_entry->val,
> -		    "QUERY_STRING") == 0 && c->querystring[0] == '\0') {
> -			bcopy(buf, c->querystring, val_len);
> -			c->querystring[val_len] = '\0';
> -		}
> -		if (val_len < GOTWEBD_MAXTEXT && strcmp(env_entry->val,
> -		    "HTTP_HOST") == 0 && c->http_host[0] == '\0') {
> -
>  			/*
>  			 * lazily get subdomain
>  			 * will only get domain if no subdomain exists
>  			 * this can still work if gotweb server name is the same
>  			 */
> -			sd = strchr(buf, '.');
> +			sd = strchr(c->http_host, '.');
>  			if (sd)
>  				*sd = '\0';
> -
> -			bcopy(buf, c->http_host, val_len);
> -			c->http_host[val_len] = '\0';
>  		}
> -		if (val_len < MAX_SCRIPT_NAME && strcmp(env_entry->val,
> -		    "SCRIPT_NAME") == 0 && c->script_name[0] == '\0') {
> -			bcopy(dr_buf, c->script_name, val_len);
> -			c->script_name[val_len] = '\0';
> -		}
> -		if (val_len < MAX_SERVER_NAME && strcmp(env_entry->val,
> -		    "SERVER_NAME") == 0 && c->server_name[0] == '\0') {
> -			bcopy(buf, c->server_name, val_len);
> -			c->server_name[val_len] = '\0';
> -		}
> -		env_entry->val[name_len] = '=';
>  
> -		bcopy(buf, (env_entry->val) + name_len + 1, val_len);
> -		buf += val_len;
> -		n -= val_len;
> +		if (c->script_name[0] == '\0' &&
> +		    val_len < MAX_SCRIPT_NAME &&
> +		    name_len == 11 &&
> +		    strncmp(buf, "SCRIPT_NAME", 11) == 0)
> +			strncpy(c->script_name, val, val_len);
>  
> -		SLIST_INSERT_HEAD(&c->env, env_entry, entry);
> -		log_debug("env[%d], %s", c->env_count, env_entry->val);
> -		c->env_count++;
> +		if (c->server_name[0] == '\0' &&
> +		    val_len < MAX_SERVER_NAME &&
> +		    name_len == 11 &&
> +		    strncmp(buf, "SERVER_NAME", 11) == 0)
> +			strncpy(c->server_name, val, val_len);
> +
> +		buf += name_len + val_len;
> +		n -= name_len - val_len;
>  	}
>  }
>  
> blob - ae08f3df1ae9fb58a5fac19a6fcfd02a1aad9247
> file + gotwebd/gotwebd.h
> --- gotwebd/gotwebd.h
> +++ gotwebd/gotwebd.h
> @@ -222,9 +222,6 @@ struct request {
>  	char				 script_name[MAX_SCRIPT_NAME];
>  	char				 server_name[MAX_SERVER_NAME];
>  
> -	struct env_head			 env;
> -	int				 env_count;
> -
>  	uint8_t				 request_started;
>  };
>