]> git.ipfire.org Git - thirdparty/FORT-validator.git/commitdiff
Patch TODO: Initialize only one CURL per thread
authorAlberto Leiva Popper <ydahhrk@gmail.com>
Wed, 29 Dec 2021 23:31:08 +0000 (17:31 -0600)
committerAlberto Leiva Popper <ydahhrk@gmail.com>
Wed, 29 Dec 2021 23:31:08 +0000 (17:31 -0600)
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.

src/http/http.c
src/http/http.h
src/state.c
src/state.h

index d9daddd6a6b8c72c1fc106d3a5d5c100381e5dd4..ad4e6c6475a5e41188a9d9e2a58891b0cae84a33 100644 (file)
@@ -3,11 +3,12 @@
 #include <errno.h>
 #include <stdio.h>
 #include <unistd.h>
-#include <curl/curl.h>
 #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;
 }
index 57616d85b3c7faa2bc6de9269e37ab51a77099ba..a81e7aaed9e7d3b39c6066f16f80d3baa2b6e1cd 100644 (file)
@@ -3,12 +3,16 @@
 
 #include <stdbool.h>
 #include <stddef.h>
+#include <curl/curl.h>
 #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 *);
index ec5df5eeab208ca842f1ba362b5ec3eaaf90717a..6bf96704d96e91ca63ff80f15faaef4e1d37e170 100644 (file)
@@ -3,6 +3,7 @@
 #include <errno.h>
 #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)
 {
index b37df7d439a74e42769c118e6ee154dbbd803b4b..9e18ae5ba568ab6c14b9b5959942ecfcaeac063d 100644 (file)
@@ -1,6 +1,7 @@
 #ifndef SRC_STATE_H_
 #define SRC_STATE_H_
 
+#include <curl/curl.h>
 #include <openssl/x509.h>
 #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 *);