[tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Feb 25 18:21:31 MSK 2019
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
More information about the Tarantool-patches
mailing list