[tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu Mar 7 17:48:20 MSK 2019
> - 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
More information about the Tarantool-patches
mailing list