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

[Kirill Shcherbatov]

Perhaps even better here is so:

diff --git a/src/httpc.c b/src/httpc.c
index b4ca6cfce..f2df069e5 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -184,23 +184,24 @@ httpc_managed_headers_mask_update(uint8_t *auto_headers_mask,
 	static struct {
 		const char *header_name;
 		uint32_t header_name_len;
-		int8_t bit_index;
 	} httpc_managed_headers[] = {
-		{"Connection",		10,	1},
-		{"Keep-Alive",		10,	2},
-		{"Content-Length",	14,	4},
-		{"Accept",		6, 	8},
+		{"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;
-		if ((*auto_headers_mask &
-		     httpc_managed_headers[i].bit_index) != 0)
+		int8_t bit_index = 1 << i;
+		if ((*auto_headers_mask & bit_index) != 0)
 			return -1;
-		*auto_headers_mask |= httpc_managed_headers[i].bit_index;
+		*auto_headers_mask |= bit_index;
 		return 0;
 	}
 	return 0;


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

ntent-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                       | 42 +++++++++++++++++++++++++++++++
 src/httpc.h                       |  8 ++++++
 test/app-tap/http_client.test.lua | 12 ++++++++-
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/src/httpc.c b/src/httpc.c
index 950f8b32f..f2df069e5 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -170,6 +170,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 +215,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");
diff --git a/src/httpc.h b/src/httpc.h
index dc709e6fa..d30343be1 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -109,6 +109,14 @@ 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;
 };
 
 /**
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 10a3728f8..8d1c50549 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -79,6 +79,14 @@ 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(2)
+    opts.headers={['Connection'] = 'Keep-Alive'}
+    local r = client.get(url, opts)
+    test:is(r.status, 200, 'simple 200')
+    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 +405,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