Commit Diff

Commit:
40a95f4f726567f73ee83d4866608f7ea0654eaa
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
gotwebd: plug leak in fcgi_parse_params fcgi_parse_params parses fastcgi parameters into a list. (This is a leftover from slowcgi where that list is later used to populate the environment of the CGI process.) However, this list is never looked at and its memory never released, so just drop it. Make the matching on fastcgi parameters name strictier by checking also that the length is the one we expect; otherwise we might pick up parameters with the same prefix string (i.e. FOO vs FOO_WITH_SUFFIX) While here turn some bcopy into memcpy and simplify some if-nesting too. Fix the reading from an un-initialized pointer that I introduced in a previous commit. ok stsp@
Actions:
Patch | Tree
commit - af7ccbc139100f0bb94b8d2ef998dedc122a18f5
commit + 40a95f4f726567f73ee83d4866608f7ea0654eaa
blob - 865c86621023df5c6c3d96bfad927b4eab09a613
blob + e77b8424231bf88c153a32dd90a19292c41dc39d
--- 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,72 @@ 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 (n < name_len + val_len)
-			return;
-
-		if ((env_entry = malloc(sizeof(struct env_val))) == NULL) {
-			log_warn("cannot malloc env_entry");
-			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 ((env_entry->val = calloc(sizeof(char), name_len + val_len +
-		    2)) == NULL) {
-			log_warn("cannot allocate env_entry->val");
-			free(env_entry);
+		if (n < name_len + val_len)
 			return;
-		}
 
-		bcopy(buf, env_entry->val, name_len);
-		buf += name_len;
-		n -= name_len;
+		val = buf + name_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);
+		if (c->querystring[0] == '\0' &&
+		    val_len < MAX_QUERYSTRING &&
+		    name_len == 12 &&
+		    strncmp(buf, "QUERY_STRING", 12) == 0) {
+			memcpy(c->querystring, val, val_len);
 			c->querystring[val_len] = '\0';
 		}
-		if (val_len < GOTWEBD_MAXTEXT && strcmp(env_entry->val,
-		    "HTTP_HOST") == 0 && c->http_host[0] == '\0') {
 
+		if (c->http_host[0] == '\0' &&
+		    val_len < GOTWEBD_MAXTEXT &&
+		    name_len == 9 &&
+		    strncmp(buf, "HTTP_HOST", 9) == 0) {
+			memcpy(c->http_host, val, val_len);
+			c->http_host[val_len] = '\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);
+
+		if (c->script_name[0] == '\0' &&
+		    val_len < MAX_SCRIPT_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SCRIPT_NAME", 11) == 0) {
+			memcpy(c->script_name, val, 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);
+
+		if (c->server_name[0] == '\0' &&
+		    val_len < MAX_SERVER_NAME &&
+		    name_len == 11 &&
+		    strncmp(buf, "SERVER_NAME", 11) == 0) {
+			memcpy(c->server_name, val, 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;
-
-		SLIST_INSERT_HEAD(&c->env, env_entry, entry);
-		log_debug("env[%d], %s", c->env_count, env_entry->val);
-		c->env_count++;
+		buf += name_len + val_len;
+		n -= name_len - val_len;
 	}
 }
 
blob - bf762e12dd70fa07bb83c88d023e1ebb29d35f27
blob + ac7f962b1abdcfe1c535e8bc210c9f619e7a2cbc
--- 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;
 };