From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers References: <9099f8a339c278e3a5dac683923d13c7ee470ce9.1550761676.git.kshcherbatov@tarantool.org> <80130a1b-84ad-5f90-9422-95bb0a1e67cb@tarantool.org> <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org> <20190307135135.dner6r66szejw6lx@esperanza> From: Kirill Shcherbatov Message-ID: <22400748-d82b-16ab-5809-68440cbfb1ef@tarantool.org> Date: Thu, 7 Mar 2019 17:48:20 +0300 MIME-Version: 1.0 In-Reply-To: <20190307135135.dner6r66szejw6lx@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: > - A space after a colon (:) isn't mandatory according to the http > protocol, e.g. "content-length:10" is a valid header. Done. > - I don't quite like string literal duplication. Can we do without it? > Use strlen, may be? The compiler should resolve it at compile time > AFAIK. We need to compare strlen() characters, not compare whole strings; I don't like to calculate it each time. As we disscused, let's keep it > > - Checking "content-length" doesn't make much sense IMO. No-one in > their right mind will overwrite it after setting a body. We only > need to handle "accept", "connection", "keep-alive". Done. > I don't like how httpc internal API now looks like: one should set > headers before the method/url and keepalive options, otherwise he/she > risks loosing them. And there isn't a word about this idiosyncrasy > anywhere in the comments. > > Also, splitting request construction into allocation (_new) and > method/url initialization (_set_url) looks ugly to me, because no > request makes sense without them. > > May be, we could set default headers in httpc_execute instead? No, when those headers are set, they use specific context: like size, idle etx. I've also applied Vlad's diff (tnx to Vlad). Let's ask Kirill to merge it?) ====================================================== The https library intelligently sets the headers "Accept", "Content-Length", "Connection", "Keep-Alive: timeout". However, when the user explicitly specified them in the header options section of the call argument, they could be written to the HTTP request twice. Was introduced service http c_managed_header_mask mask in which the bits corresponding to the http auto-manager headers, set 1 when referenced. When the bit is set, the new value is not applied. In this way, we prefer user-defined value for all httpc-managed headers. Closes #3955 --- src/httpc.c | 76 ++++++++++++++++++++++++++----- src/httpc.h | 23 ++++++++-- src/lua/httpc.c | 55 ++++++++++++---------- test/app-tap/http_client.test.lua | 16 ++++++- 4 files changed, 132 insertions(+), 38 deletions(-) diff --git a/src/httpc.c b/src/httpc.c index 950f8b32f..ab745e26b 100644 --- a/src/httpc.c +++ b/src/httpc.c @@ -33,6 +33,7 @@ #include #include +#include #include "fiber.h" #include "errinj.h" @@ -95,8 +96,7 @@ httpc_env_destroy(struct httpc_env *ctx) } struct httpc_request * -httpc_request_new(struct httpc_env *env, const char *method, - const char *url) +httpc_request_new(struct httpc_env *env) { struct httpc_request *req = mempool_alloc(&env->req_pool); if (req == NULL) { @@ -109,9 +109,17 @@ httpc_request_new(struct httpc_env *env, const char *method, region_create(&req->resp_headers, &cord()->slabc); region_create(&req->resp_body, &cord()->slabc); - if (curl_request_create(&req->curl_request) != 0) + if (curl_request_create(&req->curl_request) != 0) { + mempool_free(&env->req_pool, req); return NULL; + } + ibuf_create(&req->body, &cord()->slabc, 1); + return req; +} +int +httpc_set_url(struct httpc_request *req, const char *method, const char *url) +{ if (strcmp(method, "GET") == 0) { curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L); } else if (strcmp(method, "HEAD") == 0) { @@ -130,7 +138,7 @@ httpc_request_new(struct httpc_env *env, const char *method, curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDSIZE, 0); curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); if (httpc_set_header(req, "Accept: */*") < 0) - goto error; + return -1; } else { curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); } @@ -146,13 +154,7 @@ httpc_request_new(struct httpc_env *env, const char *method, curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L); curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); - - ibuf_create(&req->body, &cord()->slabc, 1); - - return req; -error: - mempool_free(&env->req_pool, req); - return NULL; + return 0; } void @@ -170,6 +172,54 @@ httpc_request_delete(struct httpc_request *req) mempool_free(&req->env->req_pool, req); } +/** + * Update bitmask of the http request headers that httpc may set + * automatically. In case of reserved pattern is found in header, + * routine sets corresponding bit in auto_headers_mask. + * @param auto_headers_mask A bitmask of httpc-auto-managed + * headers to pointer. + * @param header A HTTP header string. + * + * @retval 0 Specified header is not auto-managed or + * corresponding bit was not set in auto_headers_mask. + * @retval -1 Otherwise. +*/ +static int +httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header) +{ + /* + * The sequence of managed headers must be sorted to + * stop scan when strcasecmp < 0. The header is expected + * to be formated with "%s: %s" pattern, so direct size + * verification is redundant. + */ + static struct { + const char *name; + int len; + } managed_headers[] = { + {"Accept:", sizeof("Accept:") - 1 }, + {"Connection:", sizeof("Connection:") - 1}, + {"Keep-Alive:", sizeof("Keep-Alive:") - 1}, + }; + int managed_headers_cnt = lengthof(managed_headers); + assert(managed_headers_cnt < + (int)sizeof(*auto_headers_mask) * CHAR_BIT); + for (int i = 0; i < managed_headers_cnt; i++) { + int rc = strncasecmp(header, managed_headers[i].name, + managed_headers[i].len); + if (rc > 0) + continue; + if (rc < 0) + return 0; + int8_t bit_index = 1 << i; + if ((*auto_headers_mask & bit_index) != 0) + return -1; + *auto_headers_mask |= bit_index; + return 0; + } + return 0; +} + int httpc_set_header(struct httpc_request *req, const char *fmt, ...) { @@ -178,6 +228,10 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...) const char *header = tt_vsprintf(fmt, ap); va_end(ap); + if (httpc_set_header_bit(&req->auto_headers_mask, header) != 0) { + /* Do not add extra header to list. */ + return 0; + } struct curl_slist *l = curl_slist_append(req->headers, header); if (l == NULL) { diag_set(OutOfMemory, strlen(header), "curl", "http header"); diff --git a/src/httpc.h b/src/httpc.h index dc709e6fa..361af57e1 100644 --- a/src/httpc.h +++ b/src/httpc.h @@ -109,16 +109,22 @@ struct httpc_request { struct region resp_headers; /** buffer of body */ struct region resp_body; + /** + * A bitmask of the http request headers from among + * the headers that httpc can set automatically. + * Each bit represents some auto-managed HTTP header. + * This allows to prevent set them twice. + */ + uint8_t auto_headers_mask; }; /** * @brief Create a new HTTP request - * @param ctx - reference to context + * @param env HTTP client environment. * @return a new HTTP request object */ struct httpc_request * -httpc_request_new(struct httpc_env *env, const char *method, - const char *url); +httpc_request_new(struct httpc_env *env); /** * @brief Delete HTTP request @@ -137,6 +143,17 @@ httpc_request_delete(struct httpc_request *req); int httpc_set_header(struct httpc_request *req, const char *fmt, ...); +/** + * Set HTTP method and destination url, set service headers. + * @param req HTTP request object to update. + * @param method HTTP request method (GET, HEAD, POST, PUT...). + * @param url The network resource URL address string. + * @retval 0 On success. + * @retval -1 Otherwise. + */ +int +httpc_set_url(struct httpc_request *req, const char *method, const char *url); + /** * Sets body of request * @param req request diff --git a/src/lua/httpc.c b/src/lua/httpc.c index 0006efdf3..4449b2854 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -145,32 +145,15 @@ luaT_httpc_request(lua_State *L) if (ctx == NULL) return luaL_error(L, "can't get httpc environment"); - const char *method = luaL_checkstring(L, 2); - const char *url = luaL_checkstring(L, 3); - - struct httpc_request *req = httpc_request_new(ctx, method, url); + struct httpc_request *req = httpc_request_new(ctx); if (req == NULL) return luaT_error(L); - double timeout = TIMEOUT_INFINITY; - int max_header_name_length = HEADER_NAME_LEN; - if (lua_isstring(L, 4)) { - size_t len = 0; - const char *body = lua_tolstring(L, 4, &len); - if (len > 0 && httpc_set_body(req, body, len) != 0) { - httpc_request_delete(req); - return luaT_error(L); - } - } else if (!lua_isnil(L, 4)) { - httpc_request_delete(req); - return luaL_error(L, "fourth argument must be a string"); - } - - if (!lua_istable(L, 5)) { - httpc_request_delete(req); - return luaL_error(L, "fifth argument must be a table"); - } - + /* + * All user-defined headers must be set before calling + * other httpc module methods so that they take priority + * over the headers that httpc tries to set automatically. + */ lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { lua_pushnil(L); @@ -199,6 +182,32 @@ luaT_httpc_request(lua_State *L) } lua_pop(L, 1); + const char *method = luaL_checkstring(L, 2); + const char *url = luaL_checkstring(L, 3); + if (httpc_set_url(req, method, url) != 0) { + httpc_request_delete(req); + return luaT_error(L); + } + + double timeout = TIMEOUT_INFINITY; + int max_header_name_length = HEADER_NAME_LEN; + if (lua_isstring(L, 4)) { + size_t len = 0; + const char *body = lua_tolstring(L, 4, &len); + if (len > 0 && httpc_set_body(req, body, len) != 0) { + httpc_request_delete(req); + return luaT_error(L); + } + } else if (!lua_isnil(L, 4)) { + httpc_request_delete(req); + return luaL_error(L, "fourth argument must be a string"); + } + + if (!lua_istable(L, 5)) { + httpc_request_delete(req); + return luaL_error(L, "fifth argument must be a table"); + } + lua_getfield(L, 5, "ca_path"); if (!lua_isnil(L, -1)) httpc_set_ca_path(req, lua_tostring(L, -1)); diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 8a98d1e92..97326bf12 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -79,6 +79,18 @@ local function test_http_client(test, url, opts) test:is(r.status, 200, 'request') end +-- +-- gh-3955: Fix httpc auto-managed headers. +-- +local function test_http_client_headers_redefine(test, url, opts) + test:plan(3) + opts.headers={['Connection'] = 'Keep-Alive', ['Accept'] = 'text/html'} + local r = client.get(url, opts) + test:is(r.status, 200, 'simple 200') + test:is(r.headers["accept"], 'text/html', 'redefined Accept header') + test:is(r.headers["connection"], 'Keep-Alive', 'redefined Connection header') +end + local function test_cancel_and_errinj(test, url, opts) test:plan(3) local ch = fiber.channel(1) @@ -471,9 +483,11 @@ local function test_concurrent(test, url, opts) end function run_tests(test, sock_family, sock_addr) - test:plan(10) + test:plan(11) local server, url, opts = start_server(test, sock_family, sock_addr) test:test("http.client", test_http_client, url, opts) + test:test("http.client headers redefine", test_http_client_headers_redefine, + url, opts) test:test("cancel and errinj", test_cancel_and_errinj, url .. 'long_query', opts) test:test("basic http post/get", test_post_and_get, url, opts) test:test("errors", test_errors) -- 2.21.0