From: Alberto Leiva Popper Date: Wed, 29 Dec 2021 23:31:08 +0000 (-0600) Subject: Patch TODO: Initialize only one CURL per thread X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=3c01104b60c0695902ac250d931e6cc7f000f577;p=thirdparty%2FFORT-validator.git Patch TODO: Initialize only one CURL per thread Fort was reinitializing the CURL object on every HTTP download. libcurl's documentation recommends against this: You need one handle for each easy session you want to perform. Basically, you should use one handle for every thread you plan to use for transferring. (...) If you then want to transfer another file, the handle is ready to be used again. Mind you, it is even preferred that you re-use an existing handle if you intend to make another transfer. libcurl will then attempt to re-use the previous connection. The CURL was moved to the thread state variable, is initialized once per thread, per validation cycle, and is employed in every HTTP transfer. --- diff --git a/src/http/http.c b/src/http/http.c index d9daddd6..ad4e6c64 100644 --- a/src/http/http.c +++ b/src/http/http.c @@ -3,11 +3,12 @@ #include #include #include -#include #include "common.h" #include "config.h" #include "file.h" #include "log.h" +#include "state.h" +#include "thread_var.h" struct curl_args { CURL *curl; @@ -111,14 +112,14 @@ setopt_writedata(CURL *curl, struct write_callback_arg *arg) } } -static int -http_easy_init(struct curl_args *handler, long ims) +CURL * +curl_create(void) { CURL *result; result = curl_easy_init(); if (result == NULL) - return pr_enomem(); + return NULL; setopt_str(result, CURLOPT_USERAGENT, config_get_http_user_agent()); @@ -155,23 +156,18 @@ http_easy_init(struct curl_args *handler, long ims) */ setopt_long(result, CURLOPT_FAILONERROR, 1L); - /* Refer to its error buffer */ - setopt_str(result, CURLOPT_ERRORBUFFER, handler->errbuf); - /* Prepare for multithreading, avoid signals */ setopt_long(result, CURLOPT_NOSIGNAL, 1L); - if (ims > 0) { - /* Set "If-Modified-Since" header */ - setopt_long(result, CURLOPT_TIMEVALUE, ims); - setopt_long(result, CURLOPT_TIMECONDITION, - CURL_TIMECOND_IFMODSINCE); - } - - handler->curl = result; return 0; } +void +curl_destroy(CURL *curl) +{ + curl_easy_cleanup(curl); +} + static char const * curl_err_string(struct curl_args *handler, CURLcode res) { @@ -218,42 +214,44 @@ handle_http_response_code(long http_code) * only if we don't need to retry. */ static int -do_single_http_get(struct curl_args *handler, char const *src, char const *dst) +do_single_http_get(CURL *curl, char const *src, char const *dst) { - struct write_callback_arg args; + struct curl_args cargs; + struct write_callback_arg wargs; CURLcode res; long http_code; - args.total_bytes = 0; - handler->errbuf[0] = 0; - setopt_str(handler->curl, CURLOPT_URL, src); - setopt_writedata(handler->curl, &args); + cargs.curl = curl; + cargs.errbuf[0] = 0; + wargs.total_bytes = 0; + setopt_writedata(curl, &wargs); + setopt_str(curl, CURLOPT_ERRORBUFFER, cargs.errbuf); pr_val_debug("HTTP GET: %s", src); - args.error = file_write(dst, &args.dst); - if (args.error) - return args.error; + wargs.error = file_write(dst, &wargs.dst); + if (wargs.error) + return wargs.error; - res = curl_easy_perform(handler->curl); + res = curl_easy_perform(curl); - file_close(args.dst); + file_close(wargs.dst); - pr_val_debug("Done. Total bytes transferred: %zu", args.total_bytes); + pr_val_debug("Done. Total bytes transferred: %zu", wargs.total_bytes); - if (args.error == EFBIG) { + if (wargs.error == EFBIG) { pr_val_err("File too big (read: %zu bytes). Rejecting.", - args.total_bytes); + wargs.total_bytes); return EFBIG; } - args.error = get_http_response_code(handler, &http_code, src); - if (args.error) - return args.error; + wargs.error = get_http_response_code(&cargs, &http_code, src); + if (wargs.error) + return wargs.error; if (res != CURLE_OK) { pr_val_err("Error requesting URL %s: %s. (HTTP code: %ld)", - src, curl_err_string(handler, res), http_code); + src, curl_err_string(&cargs, res), http_code); switch (res) { case CURLE_FILESIZE_EXCEEDED: @@ -292,17 +290,11 @@ do_single_http_get(struct curl_args *handler, char const *src, char const *dst) return 0; } -static void -http_easy_cleanup(struct curl_args *handler) -{ - curl_easy_cleanup(handler->curl); -} - /* Retries in accordance with configuration limits. */ static int -retry_until_done(char const *remote, char const *local, - struct curl_args *handler) +retry_until_done(char const *remote, char const *local, long ims) { + CURL *curl; unsigned int attempt; unsigned int max_attempts; unsigned int retry_interval; @@ -312,13 +304,24 @@ retry_until_done(char const *remote, char const *local, if (error) return error; + curl = validation_curl(state_retrieve()); max_attempts = config_get_http_retry_count() + 1; retry_interval = config_get_http_retry_interval(); + setopt_str(curl, CURLOPT_URL, remote); + if (ims > 0) { + setopt_long(curl, CURLOPT_TIMEVALUE, ims); + setopt_long(curl, CURLOPT_TIMECONDITION, + CURL_TIMECOND_IFMODSINCE); + } else { + setopt_long(curl, CURLOPT_TIMEVALUE, 0); + setopt_long(curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_NONE); + } + for (attempt = 1; true; attempt++) { pr_val_debug("HTTP GET attempt %u (out of %u)...", attempt, max_attempts); - error = do_single_http_get(handler, remote, local); + error = do_single_http_get(curl, remote, local); if (error == 0) return 0; if (error != EAGAIN) @@ -357,7 +360,6 @@ http_get(struct rpki_uri *uri, long ims) { static char const *TMP_SUFFIX = "_tmp"; - struct curl_args handler; char *tmp_file; int error; @@ -366,27 +368,20 @@ http_get(struct rpki_uri *uri, long ims) return ENOTCHANGED; } - /* TODO (aaaa) this is reusable. Move to the thread. */ - error = http_easy_init(&handler, ims); - if (error) - return error; - /* * We will write the file into a temporal location first. * This will prevent us from overriding the existing file, which is * going to be a problem if the download turns out to fail. */ tmp_file = malloc(strlen(uri_get_local(uri)) + strlen(TMP_SUFFIX) + 1); - if (tmp_file == NULL) { - error = pr_enomem(); - goto free_handler; - } + if (tmp_file == NULL) + return pr_enomem(); strcpy(tmp_file, uri_get_local(uri)); strcat(tmp_file, TMP_SUFFIX); - error = retry_until_done(uri_get_global(uri), tmp_file, &handler); + error = retry_until_done(uri_get_global(uri), tmp_file, ims); if (error) - goto free_tmp; + goto end; if (rename(tmp_file, uri_get_local(uri)) == -1) { error = errno; @@ -394,10 +389,7 @@ http_get(struct rpki_uri *uri, long ims) tmp_file, uri_get_local(uri)); } -free_tmp: - free(tmp_file); -free_handler: - http_easy_cleanup(&handler); +end: free(tmp_file); return error; } @@ -408,15 +400,15 @@ free_handler: int http_direct_download(char const *remote, char const *dest) { - struct curl_args curl; + CURL *curl; int error; - error = http_easy_init(&curl, 0L); - if (error) - return error; + curl = curl_create(); + if (curl == NULL) + return pr_enomem(); error = do_single_http_get(&curl, remote, dest); - http_easy_cleanup(&curl); + curl_destroy(&curl); return error; } diff --git a/src/http/http.h b/src/http/http.h index 57616d85..a81e7aae 100644 --- a/src/http/http.h +++ b/src/http/http.h @@ -3,12 +3,16 @@ #include #include +#include #include "types/uri.h" /* Init on the main process */ int http_init(void); void http_cleanup(void); +CURL *curl_create(void); +void curl_destroy(CURL *); + int http_get(struct rpki_uri *, long); int http_direct_download(char const *, char const *); diff --git a/src/state.c b/src/state.c index ec5df5ee..6bf96704 100644 --- a/src/state.c +++ b/src/state.c @@ -3,6 +3,7 @@ #include #include "log.h" #include "thread_var.h" +#include "http/http.h" #include "rpp/rpp_dl_status.h" /** @@ -15,6 +16,9 @@ struct validation { struct tal *tal; + /* HTTP retriever. Reused because the documentation recommends it. */ + CURL *curl; + struct x509_data { /** https://www.openssl.org/docs/man1.1.1/man3/X509_STORE_load_locations.html */ X509_STORE *store; @@ -98,10 +102,16 @@ validation_prepare(struct tal *tal, state->tal = tal; + state->curl = curl_create(); + if (state->curl == NULL) { + error = pr_enomem(); + goto revert_state; + } + state->x509_data.store = X509_STORE_new(); if (!state->x509_data.store) { error = val_crypto_err("X509_STORE_new() returned NULL"); - goto revert_state; + goto revert_curl; } params = X509_VERIFY_PARAM_new(); @@ -134,6 +144,8 @@ revert_params: X509_VERIFY_PARAM_free(params); revert_store: X509_STORE_free(state->x509_data.store); +revert_curl: + curl_destroy(state->curl); revert_state: free(state); return error; @@ -150,6 +162,7 @@ validation_destroy(void) certstack_destroy(state->certstack); X509_VERIFY_PARAM_free(state->x509_data.params); X509_STORE_free(state->x509_data.store); + curl_destroy(state->curl); free(state); } @@ -159,6 +172,12 @@ validation_tal(struct validation *state) return state->tal; } +CURL * +validation_curl(struct validation *state) +{ + return state->curl; +} + X509_STORE * validation_store(struct validation *state) { diff --git a/src/state.h b/src/state.h index b37df7d4..9e18ae5b 100644 --- a/src/state.h +++ b/src/state.h @@ -1,6 +1,7 @@ #ifndef SRC_STATE_H_ #define SRC_STATE_H_ +#include #include #include "cert_stack.h" #include "validation_handler.h" @@ -12,6 +13,7 @@ int validation_prepare(struct tal *, struct validation_handler *); void validation_destroy(); struct tal *validation_tal(struct validation *); +CURL *validation_curl(struct validation *); X509_STORE *validation_store(struct validation *); struct cert_stack *validation_certstack(struct validation *);