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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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