Skip to content

Commit

Permalink
Fix active req handling (#244)
Browse files Browse the repository at this point in the history 
* fix own cert chain refcounting(openssl)
* fix http client close during req callbacks
* cannot free req during parser callback
  • Loading branch information
@ekoby
ekoby authored Oct 8, 2024
1 parent 37699c2 commit 927fc44
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 62 deletions.
1 change: 1 addition & 0 deletions include/tlsuv/http.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ struct tlsuv_http_s {
um_header_list headers;

int connected;
bool keepalive;
tlsuv_src_t *src;
bool own_src;

Expand Down
97 changes: 42 additions & 55 deletions src/http.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,6 @@ extern tls_context *get_default_tls(void);

static void http_read_cb(uv_link_t *link, ssize_t nread, const uv_buf_t *buf);

static int http_status_cb(llhttp_t *parser, const char *status, size_t len);

static int http_message_cb(llhttp_t *parser);

static int http_body_cb(llhttp_t *parser, const char *body, size_t len);

static int http_header_field_cb(llhttp_t *parser, const char *f, size_t len);

static int http_header_value_cb(llhttp_t *parser, const char *v, size_t len);

static int http_headers_complete_cb(llhttp_t *p);

static void fail_active_request(tlsuv_http_t *c, int code, const char *msg);

static void close_connection(tlsuv_http_t *c);
Expand Down Expand Up @@ -81,51 +69,35 @@ static void http_read_cb(uv_link_t *link, ssize_t nread, const uv_buf_t *buf) {

close_connection(c);
uv_async_send(&c->proc);
if (buf && buf->base) {
tlsuv__free(buf->base);
}
return;
}

if (c->active != NULL) {

if (nread > 0) {
if (http_req_process(c->active, buf->base, nread) < 0) {
} else if (nread > 0) {
if (c->active != NULL) {
tlsuv_http_req_t *ar = c->active;
if (http_req_process(ar, buf->base, nread) < 0) {
UM_LOG(WARN, "failed to parse HTTP response");
fail_active_request(c, UV_EINVAL, "failed to parse HTTP response");
close_connection(c);
tlsuv__free(buf->base);
return;
}
}

if (c->active->state == completed) {
tlsuv_http_req_t *hr = c->active;
c->active = NULL;

bool keep_alive = true;
const char *keep_alive_hdr = tlsuv_http_resp_header(&hr->resp, "Connection");
if (strcmp(hr->resp.http_version, "1.1") == 0) {
if (keep_alive_hdr && strcasecmp("close", keep_alive_hdr) == 0)
keep_alive = false;
} else if (strcmp(hr->resp.http_version, "1.0") == 0) {
keep_alive = keep_alive_hdr && strcasecmp("keep-alive", keep_alive_hdr) == 0;
} else {
UM_LOG(WARN, "unexpected HTTP version(%s)", hr->resp.http_version);
keep_alive = false;
}
if (ar->state == completed) {
bool keepalive = c->keepalive;
const char *keep_alive_hdr = tlsuv_http_resp_header(&ar->resp, "Connection");
if (keep_alive_hdr) {
keepalive = strcasecmp(keep_alive_hdr, "close") != 0;
}

http_req_free(hr);
tlsuv__free(hr);
c->active = NULL;
http_req_free(ar);
tlsuv__free(ar);

if (!keep_alive) {
close_connection(c);
} else {
uv_async_send(&c->proc);
if (keepalive) {
uv_async_send(&c->proc);
} else {
close_connection(c);
}
}
} else {
UM_LOG(ERR, "received %zd bytes without active request", nread);
}
} else if (nread > 0) {
UM_LOG(ERR, "received %zd bytes without active request", nread);
}

if (buf && buf->base) {
Expand All @@ -148,15 +120,24 @@ static void clear_req_body(tlsuv_http_req_t *req, int code) {
}

static void fail_active_request(tlsuv_http_t *c, int code, const char *msg) {
if (c->active != NULL && c->active->resp_cb != NULL) {
if (c->active == NULL) return;

// this is called from active request callback
if (c->active->state == completed) return;

if (c->active->resp_cb != NULL) {
c->active->resp.code = code;
c->active->resp.status = tlsuv__strdup(msg);
c->active->resp_cb(&c->active->resp, c->active->data);
clear_req_body(c->active, code);
http_req_free(c->active);
tlsuv__free(c->active);
c->active = NULL;
c->active->resp_cb = NULL;
} else if (c->active->resp.body_cb != NULL) {
c->active->resp.body_cb(c->active, NULL, code);
}

clear_req_body(c->active, code);
http_req_free(c->active);
tlsuv__free(c->active);
c->active = NULL;
}

static void fail_all_requests(tlsuv_http_t *c, int code, const char *msg) {
Expand Down Expand Up @@ -395,6 +376,11 @@ static void process_requests(uv_async_t *ar) {
if (c->active == NULL && !STAILQ_EMPTY(&c->requests)) {
c->active = STAILQ_FIRST(&c->requests);
STAILQ_REMOVE_HEAD(&c->requests, _next);

// if not keepalive close connection before next request
if (!c->keepalive) {
close_connection(c);
}
}

if (c->active == NULL) {
Expand Down Expand Up @@ -656,8 +642,9 @@ int http_req_cancel_err(tlsuv_http_t *clt, tlsuv_http_req_t *req, int error, con
req->resp.status = tlsuv__strdup(msg ? msg : uv_strerror(error));
clear_req_body(req, req->resp.code);

if (req->state < headers_received) { // resp_cb has not been called yet
req->resp_cb(&r->resp, r->data);
if (req->state < headers_received && req->resp_cb) { // resp_cb has not been called yet
req->resp_cb(&req->resp, req->data);
req->resp_cb = NULL;
} else if (req->resp.body_cb) {
req->resp.body_cb(req, NULL, req->resp.code);
}
Expand Down
6 changes: 6 additions & 0 deletions src/http_req.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,15 +361,21 @@ static int http_status_cb(llhttp_t *parser, const char *status, size_t len) {
tlsuv_http_req_t *r = parser->data;
r->resp.code = (int) parser->status_code;
snprintf(r->resp.http_version, sizeof(r->resp.http_version), "%1d.%1d", parser->http_major, parser->http_minor);
if (r->client) {
r->client->keepalive = !(parser->http_major == 1 && parser->http_minor == 0);
}
r->resp.status = tlsuv__calloc(1, len+1);
strncpy(r->resp.status, status, len);
return 0;
}



static int http_message_cb(llhttp_t *parser) {
UM_LOG(VERB, "message complete");
tlsuv_http_req_t *r = parser->data;
r->state = completed;

if (r->resp.body_cb) {
if (r->inflater == NULL || um_inflate_state(r->inflater) == 1) {
r->resp.body_cb(r, NULL, UV_EOF);
Expand Down
12 changes: 5 additions & 7 deletions src/openssl/engine.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -719,16 +719,14 @@ if ((op) != 1) { \
static X509* tls_set_cert_internal (SSL_CTX* ssl, X509_STORE *store) {
STACK_OF(X509_OBJECT) *certs = X509_STORE_get0_objects(store);
X509 *crt = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(certs, 0));
X509_up_ref(crt);
SSL_CTX_use_certificate(ssl, crt);

// rest of certs go to chain
if (sk_X509_OBJECT_num(certs) > 1) {
X509_STORE *chain = X509_STORE_new();
// skip first one
for (int i = 1; i < sk_X509_OBJECT_num(certs); i++) {
X509_STORE_add_cert(chain, X509_OBJECT_get0_X509(sk_X509_OBJECT_value(certs, i)));
}
SSL_CTX_set0_chain_cert_store(ssl, chain);
for (int i = 1; i < sk_X509_OBJECT_num(certs); i++) {
X509 *x509 = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(certs, i));
X509_up_ref(x509);
SSL_CTX_add_extra_chain_cert(ssl, x509);
}
return crt;
}
Expand Down
25 changes: 25 additions & 0 deletions tests/http_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ TEST_CASE("conn failures", "[http]") {
REQUIRE(resp.code == UV_ECONNREFUSED);

tlsuv_http_close(&clt, nullptr);
test.run();
}


Expand Down Expand Up @@ -282,6 +283,7 @@ TEST_CASE("http_tests", "[http]") {
}
tlsuv_http_close(&clt, nullptr);
tlsuv_set_global_connector(nullptr);
test.run();
}

#if defined(HSM_LIB)
Expand Down Expand Up @@ -316,6 +318,7 @@ TEST_CASE("pkcs11_client_cert_test","[http]") {
tlsuv_http_close(&clt, nullptr);
if (tls)
tls->free_ctx(tls);
test.run();
}
#endif

Expand Down Expand Up @@ -423,11 +426,13 @@ TEST_CASE("client_cert_test","[http]") {
CHECK(resp.resp_body_end_called);
CHECK(resp.body == "you are 'CN=BadSSL Client Certificate,O=BadSSL,L=San Francisco,ST=California,C=US' by CN=BadSSL Client Root Certificate Authority,O=BadSSL,L=San Francisco,ST=California,C=US");
c->free(c);
test.run();
}

tlsuv_http_close(&clt, nullptr);
if (tls)
tls->free_ctx(tls);
test.run();
}

const int ONE_SECOND = 1000000;
Expand Down Expand Up @@ -470,6 +475,7 @@ TEST_CASE("client_idle_test","[http]") {

tlsuv_http_close(&clt, nullptr);
}
test.run();
}

// hidden test
Expand Down Expand Up @@ -511,6 +517,7 @@ TEST_CASE("server_idle_close","[.]") {

tlsuv_http_close(&clt, nullptr);
}
test.run();
}

TEST_CASE("basic_test", "[http]") {
Expand Down Expand Up @@ -538,6 +545,7 @@ TEST_CASE("basic_test", "[http]") {
tlsuv_http_close(&clt, nullptr);

tlsuv_set_global_connector(nullptr);
test.run();
}

TEST_CASE("invalid CA", "[http]") {
Expand All @@ -555,6 +563,7 @@ TEST_CASE("invalid CA", "[http]") {
}

tlsuv_http_close(&clt, nullptr);
test.run();
}


Expand All @@ -573,6 +582,7 @@ TEST_CASE("http_prefix", "[http]") {
CHECK_THAT(resp.headers["Content-Length"], Equals("256"));

tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("http_prefix_after", "[http]") {
Expand All @@ -592,6 +602,7 @@ TEST_CASE("http_prefix_after", "[http]") {
CHECK_THAT(resp.headers["Content-Length"], Equals("256"));

tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("content_length_test", "[http]") {
Expand Down Expand Up @@ -630,6 +641,7 @@ TEST_CASE("content_length_test", "[http]") {
}

tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("multiple requests", "[http]") {
Expand Down Expand Up @@ -685,6 +697,7 @@ TEST_CASE("multiple requests", "[http]") {
}

tlsuv_http_close(&clt, nullptr);
test.run();
}

// test proper client->engine cleanup between requests
Expand All @@ -708,6 +721,7 @@ TEST_CASE("TLS reconnect", "[http]") {
CHECK(resp2.code == 200);

tlsuv_http_close(&clt, nullptr);
test.run();
}

typedef struct verify_ctx_s {
Expand Down Expand Up @@ -751,6 +765,7 @@ TEST_CASE("large POST(GH-87)", "[http][gh-87]") {

tlsuv_http_close(&clt, nullptr);
free(buf);
test.run();
}

TEST_CASE("TLS verify with JWT", "[http]") {
Expand Down Expand Up @@ -802,6 +817,7 @@ TEST_CASE("TLS verify with JWT", "[http]") {

free(vtx.sig);
tls->free_ctx(tls);
test.run();
}

TEST_CASE("TLS to IP address", "[http]") {
Expand All @@ -822,6 +838,7 @@ TEST_CASE("TLS to IP address", "[http]") {
CHECK(resp.headers["Content-Type"] == "application/dns-json");
CHECK_THAT(resp.body, Catch::Matchers::ContainsSubstring("\"Answer\":[{\"name\":\"google.com\""));
tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("connect timeout", "[http]") {
Expand All @@ -848,6 +865,7 @@ TEST_CASE("connect timeout", "[http]") {
CHECK(resp.code == UV_ETIMEDOUT);
CHECK(resp2.code == UV_ETIMEDOUT);
tlsuv_http_close(&clt, nullptr);
test.run();
}


Expand Down Expand Up @@ -876,6 +894,7 @@ TEST_CASE("HTTP gzip", "[http]") {
tlsuv_http_close(clt, [](tlsuv_http_t *c) {
delete c;
});
test.run();
}

TEST_CASE("HTTP deflate", "[http]") {
Expand All @@ -902,6 +921,7 @@ TEST_CASE("HTTP deflate", "[http]") {
std::cout << resp.req_body << std::endl;

tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("URL encode", "[http]") {
Expand Down Expand Up @@ -939,6 +959,7 @@ TEST_CASE("URL encode", "[http]") {

tlsuv_http_close(&clt, nullptr);
json_value_free(j);
test.run();
}

class testdata {
Expand Down Expand Up @@ -989,6 +1010,7 @@ TEST_CASE("form test", "[http]") {
tlsuv_http_close(&clt, nullptr);

json_value_free(jval);
test.run();
}

TEST_CASE("form test too big", "[http]") {
Expand Down Expand Up @@ -1021,6 +1043,7 @@ TEST_CASE("form test too big", "[http]") {
REQUIRE(resp.status == "form data too big");

tlsuv_http_close(&clt, nullptr);
test.run();
}

TEST_CASE("test req header too big", "[http]") {
Expand Down Expand Up @@ -1051,6 +1074,7 @@ TEST_CASE("test req header too big", "[http]") {
REQUIRE(resp.status == "request header too big");

tlsuv_http_close(&clt, nullptr);
test.run();
}
}

Expand Down Expand Up @@ -1083,6 +1107,7 @@ TEST_CASE("test request cancel", "[http]") {
CHECK(td.resp2.code == UV_ECANCELED);

tlsuv_http_close(&td.clt, nullptr);
test.run();
}

#define TEST_FIELD(f, VAL) \
Expand Down

0 comments on commit 927fc44

Please sign in to comment.