Download raw body.
gotwebd: memleak in (and small refactoring of) fcgi_parse_record
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. 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; };
gotwebd: memleak in (and small refactoring of) fcgi_parse_record