From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 96337256DE for ; Mon, 25 Feb 2019 02:53:03 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id D2gPehrv-Wph for ; Mon, 25 Feb 2019 02:53:03 -0500 (EST) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AAF3F256B0 for ; Mon, 25 Feb 2019 02:53:02 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers From: Kirill Shcherbatov References: <9099f8a339c278e3a5dac683923d13c7ee470ce9.1550761676.git.kshcherbatov@tarantool.org> <80130a1b-84ad-5f90-9422-95bb0a1e67cb@tarantool.org> Message-ID: Date: Mon, 25 Feb 2019 10:52:59 +0300 MIME-Version: 1.0 In-Reply-To: <80130a1b-84ad-5f90-9422-95bb0a1e67cb@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Vladislav Shpilevoy I've reworked code a bit to set all user-defined headers before other httpc methods call to they have a priority. ================================================ 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 | 123 +++++++++++++++++++++--------- src/httpc.h | 24 +++++- src/lua/httpc.c | 45 ++++++----- test/app-tap/http_client.test.lua | 13 +++- 4 files changed, 145 insertions(+), 60 deletions(-) diff --git a/src/httpc.c b/src/httpc.c index 950f8b32f..4685a712a 100644 --- a/src/httpc.c +++ b/src/httpc.c @@ -95,8 +95,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) { @@ -110,42 +109,7 @@ httpc_request_new(struct httpc_env *env, const char *method, region_create(&req->resp_body, &cord()->slabc); if (curl_request_create(&req->curl_request) != 0) - return NULL; - - if (strcmp(method, "GET") == 0) { - curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L); - } else if (strcmp(method, "HEAD") == 0) { - curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L); - } else if (strcmp(method, "POST") == 0 || - strcmp(method, "PUT") == 0 || - strcmp(method, "PATCH")) { - /* - * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0 - * to avoid the read callback in any cases even if user - * forgot to call httpc_set_body() for POST request. - * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html - */ - curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L); - curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDS, ""); - 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; - } else { - curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); - } - - curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url); - - curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1); - curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1); - curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION, - curl_easy_write_cb); - curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION, - curl_easy_header_cb); - 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); + goto error; ibuf_create(&req->body, &cord()->slabc, 1); @@ -170,6 +134,43 @@ 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. + * Returns -1 when header is reserved and it's bit is already set + * in auto_headers_mask; 0 otherwise, +*/ +static int +httpc_managed_headers_mask_update(uint8_t *auto_headers_mask, + const char *header) +{ + static struct { + const char *header_name; + uint32_t header_name_len; + } httpc_managed_headers[] = { + {"Connection", 10}, + {"Keep-Alive", 10}, + {"Content-Length", 14}, + {"Accept", 6}, + }; + uint32_t httpc_managed_headers_cnt = + sizeof(httpc_managed_headers)/sizeof(httpc_managed_headers[0]); + assert(httpc_managed_headers_cnt < + sizeof(*auto_headers_mask) * CHAR_BIT); + for (uint32_t i = 0; i < httpc_managed_headers_cnt; i++) { + if (strncasecmp(header, httpc_managed_headers[i].header_name, + httpc_managed_headers[i].header_name_len) != 0) + continue; + 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 +179,11 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...) const char *header = tt_vsprintf(fmt, ap); va_end(ap); + if (httpc_managed_headers_mask_update(&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"); @@ -187,6 +193,47 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...) return 0; } +int +httpc_set_resource(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) { + curl_easy_setopt(req->curl_request.easy, CURLOPT_NOBODY, 1L); + } else if (strcmp(method, "POST") == 0 || + strcmp(method, "PUT") == 0 || + strcmp(method, "PATCH")) { + /* + * Set CURLOPT_POSTFIELDS to "" and CURLOPT_POSTFIELDSSIZE 0 + * to avoid the read callback in any cases even if user + * forgot to call httpc_set_body() for POST request. + * @see https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html + */ + curl_easy_setopt(req->curl_request.easy, CURLOPT_POST, 1L); + curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDS, ""); + 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) + return -1; + } else { + curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); + } + + curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url); + + curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1); + curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1); + curl_easy_setopt(req->curl_request.easy, CURLOPT_WRITEFUNCTION, + curl_easy_write_cb); + curl_easy_setopt(req->curl_request.easy, CURLOPT_HEADERFUNCTION, + curl_easy_header_cb); + 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); + return 0; +} + int httpc_set_body(struct httpc_request *req, const char *body, size_t size) { diff --git a/src/httpc.h b/src/httpc.h index dc709e6fa..8b400ae43 100644 --- a/src/httpc.h +++ b/src/httpc.h @@ -109,16 +109,23 @@ 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. + * This allows to prevent settings such headers twice. + * Read httpc_managed_headers_mask_update definition for + * more details. + */ + 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 +144,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 request + * @param method HTTP request method (GET, HEAD, POST, PUT...) + * @param url The network resource URL address string. + * @retval 0 for success, -1 otherwise + */ +int +httpc_set_resource(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 5572b70e9..a3a311c64 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -145,12 +145,36 @@ luaT_httpc_request(lua_State *L) if (ctx == NULL) return luaL_error(L, "can't get httpc environment"); + struct httpc_request *req = httpc_request_new(ctx); + if (req == NULL) + return luaT_error(L); + + /* + * 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); + while (lua_next(L, -2) != 0) { + if (httpc_set_header(req, "%s: %s", + lua_tostring(L, -2), + lua_tostring(L, -1)) < 0) { + httpc_request_delete(req); + return luaT_error(L); + } + lua_pop(L, 1); + } + } + lua_pop(L, 1); + const char *method = luaL_checkstring(L, 2); const char *url = luaL_checkstring(L, 3); - - struct httpc_request *req = httpc_request_new(ctx, method, url); - if (req == NULL) + if (httpc_set_resource(req, method, url) != 0) { + httpc_request_delete(req); return luaT_error(L); + } double timeout = TIMEOUT_INFINITY; int max_header_name_length = HEADER_NAME_LEN; @@ -171,21 +195,6 @@ luaT_httpc_request(lua_State *L) return luaL_error(L, "fifth argument must be a table"); } - lua_getfield(L, 5, "headers"); - if (!lua_isnil(L, -1)) { - lua_pushnil(L); - while (lua_next(L, -2) != 0) { - if (httpc_set_header(req, "%s: %s", - lua_tostring(L, -2), - lua_tostring(L, -1)) < 0) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); - } - } - lua_pop(L, 1); - 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 10a3728f8..26119fa73 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -79,6 +79,15 @@ local function test_http_client(test, url, opts) test:is(r.status, 200, 'request') end +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) @@ -397,9 +406,11 @@ local function test_concurrent(test, url, opts) end function run_tests(test, sock_family, sock_addr) - test:plan(9) + test:plan(10) 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.20.1