From c7c6fc010fd0c43a45ec9abb6e0c171f4760dbb3 Mon Sep 17 00:00:00 2001 From: Jessie Murray Date: Sat, 6 Nov 2021 11:44:37 -0700 Subject: [PATCH 1/3] Avoid copying header strings for http_response http_response has an array of http_header key/value pairs, and most of the time these use constant strings that do not need to be copied and re-allocated. This change adds a flag tracking which values need to be copied, were copied and need to be freed. --- src/formats/common.c | 12 ++++----- src/formats/custom-type.c | 2 +- src/http.c | 55 +++++++++++++++++++++++---------------- src/http.h | 10 ++++++- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/formats/common.c b/src/formats/common.c index 1932143..e459a2a 100644 --- a/src/formats/common.c +++ b/src/formats/common.c @@ -85,11 +85,11 @@ format_send_reply(struct cmd *cmd, const char *p, size_t sz, const char *content resp = http_response_init(cmd->w, 200, "OK"); resp->http_version = cmd->http_version; if(cmd->filename) { - http_response_set_header(resp, "Content-Disposition", cmd->filename); + http_response_set_header(resp, "Content-Disposition", cmd->filename, HEADER_COPY_VALUE); } - http_response_set_header(resp, "Content-Type", ct); + http_response_set_header(resp, "Content-Type", ct, HEADER_COPY_VALUE); http_response_set_keep_alive(resp, 1); - http_response_set_header(resp, "Transfer-Encoding", "chunked"); + http_response_set_header(resp, "Transfer-Encoding", "chunked", HEADER_COPY_NONE); http_response_set_body(resp, p, sz); http_response_write(resp, cmd->fd); } else { @@ -109,10 +109,10 @@ format_send_reply(struct cmd *cmd, const char *p, size_t sz, const char *content } else { resp = http_response_init(cmd->w, 200, "OK"); if(cmd->filename) { - http_response_set_header(resp, "Content-Disposition", cmd->filename); + http_response_set_header(resp, "Content-Disposition", cmd->filename, HEADER_COPY_VALUE); } - http_response_set_header(resp, "Content-Type", ct); - http_response_set_header(resp, "ETag", etag); + http_response_set_header(resp, "Content-Type", ct, HEADER_COPY_VALUE); + http_response_set_header(resp, "ETag", etag, HEADER_COPY_VALUE); http_response_set_body(resp, p, sz); } resp->http_version = cmd->http_version; diff --git a/src/formats/custom-type.c b/src/formats/custom-type.c index 3fc6e08..10d0cee 100644 --- a/src/formats/custom-type.c +++ b/src/formats/custom-type.c @@ -63,7 +63,7 @@ custom_type_reply(redisAsyncContext *c, void *r, void *privdata) { /* couldn't make sense of what the client wanted. */ resp = http_response_init(cmd->w, 400, "Bad Request"); - http_response_set_header(resp, "Content-Length", "0"); + http_response_set_header(resp, "Content-Length", "0", HEADER_COPY_NONE); http_response_set_keep_alive(resp, cmd->keep_alive); http_response_write(resp, cmd->fd); diff --git a/src/http.c b/src/http.c index cd31ea0..92efdb4 100644 --- a/src/http.c +++ b/src/http.c @@ -26,23 +26,23 @@ http_response_init(struct worker *w, int code, const char *msg) { r->w = w; r->keep_alive = 0; /* default */ - http_response_set_header(r, "Server", "Webdis"); + http_response_set_header(r, "Server", "Webdis", HEADER_COPY_NONE); /* Cross-Origin Resource Sharing, CORS. */ - http_response_set_header(r, "Allow", "GET,POST,PUT,OPTIONS"); + http_response_set_header(r, "Allow", "GET,POST,PUT,OPTIONS", HEADER_COPY_NONE); /* Chrome doesn't support Allow and requires Access-Control-Allow-Methods */ - http_response_set_header(r, "Access-Control-Allow-Methods", "GET,POST,PUT,OPTIONS"); - http_response_set_header(r, "Access-Control-Allow-Origin", "*"); + http_response_set_header(r, "Access-Control-Allow-Methods", "GET,POST,PUT,OPTIONS", HEADER_COPY_NONE); + http_response_set_header(r, "Access-Control-Allow-Origin", "*", HEADER_COPY_NONE); /* According to http://www.w3.org/TR/cors/#access-control-allow-headers-response-header Access-Control-Allow-Headers cannot be a wildcard and must be set with explicit names */ - http_response_set_header(r, "Access-Control-Allow-Headers", "X-Requested-With, Content-Type, Authorization"); + http_response_set_header(r, "Access-Control-Allow-Headers", "X-Requested-With, Content-Type, Authorization", HEADER_COPY_NONE); return r; } @@ -67,7 +67,7 @@ http_response_init_with_buffer(struct worker *w, char *data, size_t data_sz, int void -http_response_set_header(struct http_response *r, const char *k, const char *v) { +http_response_set_header(struct http_response *r, const char *k, const char *v, header_copy copy) { int i, pos = r->header_count; size_t key_sz = strlen(k); @@ -77,8 +77,8 @@ http_response_set_header(struct http_response *r, const char *k, const char *v) if(strncmp(r->headers[i].key, k, key_sz) == 0) { pos = i; /* free old value before replacing it. */ - free(r->headers[i].key); - free(r->headers[i].val); + if(r->headers[i].copy == HEADER_COPY_KEY) free(r->headers[i].key); + if(r->headers[i].copy == HEADER_COPY_VALUE) free(r->headers[i].val); break; } } @@ -90,16 +90,27 @@ http_response_set_header(struct http_response *r, const char *k, const char *v) r->header_count++; } - /* copy key */ - r->headers[pos].key = calloc(key_sz + 1, 1); - memcpy(r->headers[pos].key, k, key_sz); + /* copy key if needed */ + if(copy == HEADER_COPY_KEY) { + r->headers[pos].key = calloc(key_sz + 1, 1); + memcpy(r->headers[pos].key, k, key_sz); + } else { + r->headers[pos].key = (char *)k; + } r->headers[pos].key_sz = key_sz; /* copy val */ - r->headers[pos].val = calloc(val_sz + 1, 1); - memcpy(r->headers[pos].val, v, val_sz); + if(copy == HEADER_COPY_VALUE) { + r->headers[pos].val = calloc(val_sz + 1, 1); + memcpy(r->headers[pos].val, v, val_sz); + } else { + r->headers[pos].val = (char *)v; + } r->headers[pos].val_sz = val_sz; + /* track what was copied */ + r->headers[pos].copy = copy; + if(!r->chunked && !strcmp(k, "Transfer-Encoding") && !strcmp(v, "chunked")) { r->chunked = 1; } @@ -126,8 +137,8 @@ http_response_cleanup(struct http_response *r, int fd, int success) { /* cleanup response object */ for(i = 0; i < r->header_count; ++i) { - free(r->headers[i].key); - free(r->headers[i].val); + if(r->headers[i].copy & HEADER_COPY_KEY) free(r->headers[i].key); + if(r->headers[i].copy & HEADER_COPY_VALUE) free(r->headers[i].val); } free(r->headers); @@ -206,9 +217,9 @@ http_response_write(struct http_response *r, int fd) { if(r->code == 200 && r->body) { char content_length[22]; sprintf(content_length, "%zd", r->body_len); - http_response_set_header(r, "Content-Length", content_length); + http_response_set_header(r, "Content-Length", content_length, HEADER_COPY_VALUE); } else { - http_response_set_header(r, "Content-Length", "0"); + http_response_set_header(r, "Content-Length", "0", HEADER_COPY_NONE); } } @@ -290,7 +301,7 @@ http_crossdomain(struct http_client *c) { resp->http_version = c->http_version; http_response_set_connection_header(c, resp); - http_response_set_header(resp, "Content-Type", "application/xml"); + http_response_set_header(resp, "Content-Type", "application/xml", HEADER_COPY_NONE); http_response_set_body(resp, out, sizeof(out)-1); http_response_write(resp, c->fd); @@ -317,9 +328,9 @@ void http_response_set_keep_alive(struct http_response *r, int enabled) { r->keep_alive = enabled; if(enabled) { - http_response_set_header(r, "Connection", "Keep-Alive"); + http_response_set_header(r, "Connection", "Keep-Alive", HEADER_COPY_NONE); } else { - http_response_set_header(r, "Connection", "Close"); + http_response_set_header(r, "Connection", "Close", HEADER_COPY_NONE); } } @@ -331,8 +342,8 @@ http_send_options(struct http_client *c) { resp->http_version = c->http_version; http_response_set_connection_header(c, resp); - http_response_set_header(resp, "Content-Type", "text/html"); - http_response_set_header(resp, "Content-Length", "0"); + http_response_set_header(resp, "Content-Type", "text/html", HEADER_COPY_NONE); + http_response_set_header(resp, "Content-Length", "0", HEADER_COPY_NONE); http_response_write(resp, c->fd); http_client_reset(c); diff --git a/src/http.h b/src/http.h index 8deb542..6df477c 100644 --- a/src/http.h +++ b/src/http.h @@ -7,12 +7,20 @@ struct http_client; struct worker; +typedef enum { + HEADER_COPY_NONE = 0, + HEADER_COPY_KEY = 1, + HEADER_COPY_VALUE = 2 +} header_copy; + struct http_header { char *key; size_t key_sz; char *val; size_t val_sz; + + header_copy copy; }; @@ -49,7 +57,7 @@ struct http_response * http_response_init_with_buffer(struct worker *w, char *data, size_t data_sz, int keep_alive); void -http_response_set_header(struct http_response *r, const char *k, const char *v); +http_response_set_header(struct http_response *r, const char *k, const char *v, header_copy copy); void http_response_set_body(struct http_response *r, const char *body, size_t body_len); From dc9d1b646e2d6d9daa295e23d54b3994f422bf02 Mon Sep 17 00:00:00 2001 From: Jessie Murray Date: Sat, 6 Nov 2021 12:17:49 -0700 Subject: [PATCH 2/3] Avoid re-allocating headers array in http_response Allocate the headers array once with the default number of entries sufficient for most requests, and only re-allocate if needed instead of re-allocating with each header. --- src/http.c | 32 +++++++++++++++++++++++++++++--- src/http.h | 3 ++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/http.c b/src/http.c index 92efdb4..83d764f 100644 --- a/src/http.c +++ b/src/http.c @@ -11,6 +11,14 @@ /* HTTP Response */ +#define DEFAULT_HEADERS_ARRAY_SIZE 9 + +static void +http_response_allocate_headers(struct http_response *r) { + r->headers_array_size = DEFAULT_HEADERS_ARRAY_SIZE; + r->headers = calloc(r->headers_array_size, sizeof(struct http_header)); +} + struct http_response * http_response_init(struct worker *w, int code, const char *msg) { @@ -26,6 +34,14 @@ http_response_init(struct worker *w, int code, const char *msg) { r->w = w; r->keep_alive = 0; /* default */ + /* pre-allocate array for headers */ + http_response_allocate_headers(r); + if(!r->headers) { + if(w && w->s) slog(w->s, WEBDIS_ERROR, "Failed to allocate http_response headers", 0); + free(r); + return NULL; + } + http_response_set_header(r, "Server", "Webdis", HEADER_COPY_NONE); /* Cross-Origin Resource Sharing, CORS. */ @@ -57,6 +73,14 @@ http_response_init_with_buffer(struct worker *w, char *data, size_t data_sz, int } r->w = w; + /* pre-allocate array for headers */ + http_response_allocate_headers(r); + if(!r->headers) { + if(w && w->s) slog(w->s, WEBDIS_ERROR, "Failed to allocate http_response headers", 0); + free(r); + return NULL; + } + /* provide buffer directly */ r->out = data; r->out_sz = data_sz; @@ -84,11 +108,13 @@ http_response_set_header(struct http_response *r, const char *k, const char *v, } /* extend array */ - if(pos == r->header_count) { + if(pos == r->headers_array_size) { + /* FIXME: allocation could fail */ r->headers = realloc(r->headers, - sizeof(struct http_header)*(r->header_count + 1)); - r->header_count++; + sizeof(struct http_header)*(r->headers_array_size + 1)); + r->headers_array_size++; } + r->header_count++; /* copy key if needed */ if(copy == HEADER_COPY_KEY) { diff --git a/src/http.h b/src/http.h index 6df477c..0310f1d 100644 --- a/src/http.h +++ b/src/http.h @@ -32,7 +32,8 @@ struct http_response { const char *msg; struct http_header *headers; - int header_count; + int header_count; /* actual count in array */ + int headers_array_size; /* allocated size */ const char *body; size_t body_len; From 7ce6d497c1cab2bed8f78f6eda7d508c644f5754 Mon Sep 17 00:00:00 2001 From: Jessie Murray Date: Tue, 16 Nov 2021 17:48:16 -0800 Subject: [PATCH 3/3] Add HEADER_CHECK_DUPE to bypass duplicate check Almost all header entries are guaranteed to be added only once, so we don't need to check for duplicates all the time. In the current code base only Content-Length has the potential for being added twice, and even then it seems highly unlikely. For all others, we can now bypass this check. This commit also changes the header_copy flags to be 1-bit flags, so that they can be combined. --- src/http.c | 24 +++++++++++++----------- src/http.h | 8 +++++--- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/http.c b/src/http.c index 83d764f..d750d6a 100644 --- a/src/http.c +++ b/src/http.c @@ -97,13 +97,15 @@ http_response_set_header(struct http_response *r, const char *k, const char *v, size_t key_sz = strlen(k); size_t val_sz = strlen(v); - for(i = 0; i < r->header_count; ++i) { - if(strncmp(r->headers[i].key, k, key_sz) == 0) { - pos = i; - /* free old value before replacing it. */ - if(r->headers[i].copy == HEADER_COPY_KEY) free(r->headers[i].key); - if(r->headers[i].copy == HEADER_COPY_VALUE) free(r->headers[i].val); - break; + if(copy & HEADER_CHECK_DUPE) { + for(i = 0; i < r->header_count; ++i) { + if(strncmp(r->headers[i].key, k, key_sz) == 0) { + pos = i; + /* free old value before replacing it. */ + if(r->headers[i].copy & HEADER_COPY_KEY) free(r->headers[i].key); + if(r->headers[i].copy & HEADER_COPY_VALUE) free(r->headers[i].val); + break; + } } } @@ -117,7 +119,7 @@ http_response_set_header(struct http_response *r, const char *k, const char *v, r->header_count++; /* copy key if needed */ - if(copy == HEADER_COPY_KEY) { + if(copy & HEADER_COPY_KEY) { r->headers[pos].key = calloc(key_sz + 1, 1); memcpy(r->headers[pos].key, k, key_sz); } else { @@ -126,7 +128,7 @@ http_response_set_header(struct http_response *r, const char *k, const char *v, r->headers[pos].key_sz = key_sz; /* copy val */ - if(copy == HEADER_COPY_VALUE) { + if(copy & HEADER_COPY_VALUE) { r->headers[pos].val = calloc(val_sz + 1, 1); memcpy(r->headers[pos].val, v, val_sz); } else { @@ -243,9 +245,9 @@ http_response_write(struct http_response *r, int fd) { if(r->code == 200 && r->body) { char content_length[22]; sprintf(content_length, "%zd", r->body_len); - http_response_set_header(r, "Content-Length", content_length, HEADER_COPY_VALUE); + http_response_set_header(r, "Content-Length", content_length, HEADER_COPY_VALUE | HEADER_CHECK_DUPE); } else { - http_response_set_header(r, "Content-Length", "0", HEADER_COPY_NONE); + http_response_set_header(r, "Content-Length", "0", HEADER_COPY_NONE | HEADER_CHECK_DUPE); } } diff --git a/src/http.h b/src/http.h index 0310f1d..20d8d8a 100644 --- a/src/http.h +++ b/src/http.h @@ -7,10 +7,12 @@ struct http_client; struct worker; +/* bit flags */ typedef enum { - HEADER_COPY_NONE = 0, - HEADER_COPY_KEY = 1, - HEADER_COPY_VALUE = 2 + HEADER_COPY_NONE = 0, /* don't strdup key or value */ + HEADER_COPY_KEY = 1, /* strdup key only */ + HEADER_COPY_VALUE = 2, /* strdup value only */ + HEADER_CHECK_DUPE = 4 /* replace duplicate when adding header */ } header_copy; struct http_header {