From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D410227471
	for <tarantool-patches@freelists.org>; Mon, 25 Feb 2019 10:21:34 -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 9s7KgsmuIQBP for <tarantool-patches@freelists.org>;
	Mon, 25 Feb 2019 10:21:34 -0500 (EST)
Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251])
	(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 D8CFC27118
	for <tarantool-patches@freelists.org>; Mon, 25 Feb 2019 10:21:33 -0500 (EST)
Subject: [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>
 <e33a292d-fb4d-5829-8c75-e8d279f95962@tarantool.org>
 <bb4e194d-2af1-34bf-5577-8dcce1f0027c@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Message-ID: <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org>
Date: Mon, 25 Feb 2019 18:21:31 +0300
MIME-Version: 1.0
In-Reply-To: <bb4e194d-2af1-34bf-5577-8dcce1f0027c@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: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

Hi! Thank you for review.

> 1. The email is utterly corrupted - all the indentation is lost. Please,
> do not copypaste patches into an email without proper preparations.
Ok.

> 2. In such a small function it is already clear, that you
> do not need this single 'goto error'. Just process that error
> here.
Done.

> 3. Use doxygen style.> 4. I do not understand the name. Is it supposed to be a check?
> Then, firstly, for checks we usually use '<object>_is/has_<property>'
> naming template. But anyway it is not a check, because it changes
> the mask. Come up with a better name, or use, for example, this:
> 
>      httpc_set_header_bit
/**
 * 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,
 * @param auto_headers_mask The bitmask of httpc-auto-managed
 *                          headers to pointer.
 * @param header The HTTP header string.
 * @retval 0 When specified header is not auto-managed or when
 *           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)


> 5. The code below looks like Java. Please, rename to just 'name'
> and 'len'.> 6. I do not see any point in storing lengths here, because
> anyway you can not use memcmp. Strncasecmp below will check
> for terminating zero in both parameters anyway.
It is not so, because we must compare just only prefix of specified length.
For more details, show 9th comment.

> 7. We have lengthof in trivia/utils.h.
        /*
	 * 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.
	 */
	struct {
		const char *name;
		int len;
	} managed_headers[] = {
		{"Accept: ", sizeof("Accept: ") - 1},
		{"Connection: ", sizeof("Connection: ") - 1},
		{"Content-Length: ", sizeof("Content-Length: ") - 1},
		{"Keep-Alive: ", sizeof("Keep-Alive: ") - 1},
	};
	

> 8. Please, do not use non standard types without necessity.
> https://github.com/tarantool/tarantool/issues/2161
for (int i = 0; i < managed_headers_cnt; i++) {

> 9. As I said at least 2 times for one of previous issues, strncmp does
> not work properly without additional checks. Your code will consider
> 'Acceptttttt' and 'Accept' to be equal. Please, fix that and add a test.
It is not so here, because header is a string that have strong format "%s: %s"
I've modified managed_headers array below to use "%s: " prefix.
Here It is not a bug, but feature =)

> 10. I propose you to store const headers of httpc_managed_headers in a sorted
> order. Sorted by the header names. Then, if during an iteration in the cycle
> above you see, that strncasecmp() returned < 0, you can return without
> further checks. I hope, that you understand why.
> 
> Another option is a binary search, but I think, that it is overkill here.
Implemented.

> 11. Firstly, it is a method of http_request, and should have
> prefix 'http_request_'. But as I see, it is a common style
> violation for this library, so lets turn the blind eye to it.
> 
> Secondly, personally I would like 'httpc_set_url', because it
> looks shorter. And because 'resource' confused me at a first
> glance.
> 
> Thirdly, please, move that function implementation below
> httpc_request_new to reduce diff.
Done.

> 12. Do not put references 'for details' to some internal
> functions. They likely to be changed, or renamed, or deleted,
> and your reference will be useless. Explain here everything.
	/**
	 * 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;

> 13.
> - Please, start each @<option> description with a capital letter,
>    and finish with a dot;
> 
> - Please, use separate @retval for different return values
>    description.
> 
> - Do not put double whitespaces in your comments. Here and in other
>    places too.
(I just copied the template of the next comment, they are all like that.)
/**
 * 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);

> 
>> + */
>> +int
>> +httpc_set_resource(struct httpc_request *req, const char *method,
>> +		   const char *url);
>> +
>>  /**
>>   * Sets body of request
>>   * @param req request
>> 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)
> 
> 14. Setting a new test out, please, write a comment about what this
> test tests, and put a github issue number in it. Conforming our
> test comments style.
--
-- gh-3955: fix httpc auto-managed headers
--
local function test_http_client_headers_redefine(test, url, opts)

============================================

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                       | 80 ++++++++++++++++++++++++++-----
 src/httpc.h                       | 23 +++++++--
 src/lua/httpc.c                   | 45 ++++++++++-------
 test/app-tap/http_client.test.lua | 16 ++++++-
 4 files changed, 131 insertions(+), 33 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..96740f950 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -33,6 +33,7 @@
 
 #include <assert.h>
 #include <curl/curl.h>
+#include <trivia/util.h>
 
 #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,19 @@ 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 +140,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 +156,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 +174,56 @@ 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,
+ * @param auto_headers_mask The bitmask of httpc-auto-managed
+ *                          headers to pointer.
+ * @param header The HTTP header string.
+ * @retval 0 When specified header is not auto-managed or when
+ *           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.
+	 */
+	struct {
+		const char *name;
+		int len;
+	} managed_headers[] = {
+		{"Accept: ", sizeof("Accept: ") - 1},
+		{"Connection: ", sizeof("Connection: ") - 1},
+		{"Content-Length: ", sizeof("Content-Length: ") - 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 +232,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..dc18ffb69 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 5572b70e9..d743eeb49 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_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;
@@ -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..b8573a572 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)
@@ -397,9 +409,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