Re: [tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

>  - 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 <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,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