Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed initialisation of sizeof_ssl_backend_data #2083

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1781,7 +1781,7 @@ static void llist_dtor(void *user, void *element)
static struct connectdata *allocate_conn(struct Curl_easy *data)
{
#ifdef USE_SSL
#define SSL_EXTRA + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
#define SSL_EXTRA (4 * Curl_ssl->get_sizeof_backend_data() - sizeof(long long))
#else
#define SSL_EXTRA 0
#endif
@@ -1877,11 +1877,11 @@ static struct connectdata *allocate_conn(struct Curl_easy *data)
char *p = (char *)&conn->align_data__do_not_use;
conn->ssl[0].backend = (struct ssl_backend_data *)p;
conn->ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data());
conn->proxy_ssl[0].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 2);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data() * 2);
conn->proxy_ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 3);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data() * 3);
}
#endif

@@ -701,6 +701,11 @@ static void *Curl_axtls_get_internals(struct ssl_connect_data *connssl,
return BACKEND->ssl;
}

static size_t Curl_axtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_axtls = {
{ CURLSSLBACKEND_AXTLS, "axtls" }, /* info */

@@ -710,7 +715,7 @@ const struct Curl_ssl Curl_ssl_axtls = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_axtls_get_backend_size, /* get_sizeof_backend_data */

/*
* axTLS has no global init. Everything is done through SSL and SSL_CTX
@@ -977,6 +977,11 @@ static void *Curl_cyassl_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_cyassl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_cyassl = {
{ CURLSSLBACKEND_WOLFSSL, "WolfSSL" }, /* info */

@@ -990,7 +995,7 @@ const struct Curl_ssl Curl_ssl_cyassl = {
1, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_cyassl_get_backend_size, /* get_sizeof_backend_data */

Curl_cyassl_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -2976,6 +2976,11 @@ static void *Curl_darwinssl_get_internals(struct ssl_connect_data *connssl,
return BACKEND->ssl_ctx;
}

static size_t Curl_darwinssl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_darwinssl = {
{ CURLSSLBACKEND_DARWINSSL, "darwinssl" }, /* info */

@@ -2989,7 +2994,7 @@ const struct Curl_ssl Curl_ssl_darwinssl = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_darwinssl_get_backend_size, /* get_sizeof_backend_data */

Curl_none_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -1352,6 +1352,11 @@ static void *Curl_gskit_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_gskit_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_gskit = {
{ CURLSSLBACKEND_GSKIT, "gskit" }, /* info */

@@ -1362,7 +1367,7 @@ const struct Curl_ssl Curl_ssl_gskit = {
/* TODO: convert to 1 and fix test #1014 (if need) */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_gskit_get_backend_size, /* get_sizeof_backend_data */

Curl_gskit_init, /* init */
Curl_gskit_cleanup, /* cleanup */
@@ -1805,6 +1805,11 @@ static void *Curl_gtls_get_internals(struct ssl_connect_data *connssl,
return BACKEND->session;
}

static size_t Curl_gtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_gnutls = {
{ CURLSSLBACKEND_GNUTLS, "gnutls" }, /* info */

@@ -1814,7 +1819,7 @@ const struct Curl_ssl Curl_ssl_gnutls = {
0, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_gtls_get_backend_size, /* get_sizeof_backend_data */

Curl_gtls_init, /* init */
Curl_gtls_cleanup, /* cleanup */
@@ -1039,6 +1039,11 @@ static void *Curl_mbedtls_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ssl;
}

static size_t Curl_mbedtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_mbedtls = {
{ CURLSSLBACKEND_MBEDTLS, "mbedtls" }, /* info */

@@ -1048,7 +1053,7 @@ const struct Curl_ssl Curl_ssl_mbedtls = {
1, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_mbedtls_get_backend_size, /* get_sizeof_backend_data */

Curl_mbedtls_init, /* init */
Curl_mbedtls_cleanup, /* cleanup */
@@ -2342,6 +2342,11 @@ static void *Curl_nss_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_nss_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_nss = {
{ CURLSSLBACKEND_NSS, "nss" }, /* info */

@@ -2351,7 +2356,7 @@ const struct Curl_ssl Curl_ssl_nss = {
0, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_nss_get_backend_size, /* get_sizeof_backend_data */

Curl_nss_init, /* init */
Curl_nss_cleanup, /* cleanup */
@@ -3621,6 +3621,11 @@ static void *Curl_ossl_get_internals(struct ssl_connect_data *connssl,
(void *)BACKEND->ctx : (void *)BACKEND->handle;
}

static size_t Curl_ossl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_openssl = {
{ CURLSSLBACKEND_OPENSSL, "openssl" }, /* info */

@@ -3630,7 +3635,7 @@ const struct Curl_ssl Curl_ssl_openssl = {
1, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_ossl_get_backend_size, /* get_sizeof_backend_data */

Curl_ossl_init, /* init */
Curl_ossl_cleanup, /* cleanup */
@@ -898,6 +898,11 @@ static void *Curl_polarssl_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ssl;
}

static size_t Curl_polarssl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_polarssl = {
{ CURLSSLBACKEND_POLARSSL, "polarssl" }, /* info */

@@ -907,7 +912,7 @@ const struct Curl_ssl Curl_ssl_polarssl = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_polarssl_get_backend_size, /* get_sizeof_backend_data */

Curl_polarssl_init, /* init */
Curl_polarssl_cleanup, /* cleanup */
@@ -1816,6 +1816,11 @@ static void *Curl_schannel_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ctxt->ctxt_handle;
}

static size_t Curl_schannel_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_schannel = {
{ CURLSSLBACKEND_SCHANNEL, "schannel" }, /* info */

@@ -1825,7 +1830,7 @@ const struct Curl_ssl Curl_ssl_schannel = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_schannel_get_backend_size, /* get_sizeof_backend_data */

Curl_schannel_init, /* init */
Curl_schannel_cleanup, /* cleanup */
@@ -220,7 +220,7 @@ ssl_connect_init_proxy(struct connectdata *conn, int sockindex)
conn->proxy_ssl[sockindex] = conn->ssl[sockindex];

memset(&conn->ssl[sockindex], 0, sizeof(conn->ssl[sockindex]));
memset(pbdata, 0, Curl_ssl->sizeof_ssl_backend_data);
memset(pbdata, 0, Curl_ssl->get_sizeof_backend_data());

conn->ssl[sockindex].backend = pbdata;
}
@@ -1088,6 +1088,13 @@ CURLcode Curl_none_md5sum(unsigned char *input UNUSED_PARAM,
}
#endif

static size_t Curl_multissl_get_backend_size(void)
{
if(multissl_init(NULL))
return 0;

This comment has been minimized.

Copy link
@dscho

dscho Nov 17, 2017

Contributor

Ah, see, that's why large patches are bad: I missed this particular part.

And here you suggest one possible alternative, less intrusive solution: replace the -1 in Curl_ssl_multi by 0.

However, I still would like the alternative I proposed earlier better, where multissl_init(NULL); is called from url.c just before we really need to know the size of the backend data. If you are truly concerned about performance, you can even guard that call behind a test whether the size is still equal to (size_t)-1.

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 18, 2017

Author Contributor

@dscho Do not call this tiny patch "large patch". 😃 It simply have a lot of similar lines so it is easy to miss interesting points.

Yep, initialisation could be called from many points.
However, it is not very intuitive to check size value and call TLS initialisation from url.c. I'd better to stick to one of my PRs. Either call initialisation with global initialisation (no additional is required) or use automatic initialisation (like in this PR).

This comment has been minimized.

Copy link
@dscho

dscho Nov 25, 2017

Contributor

It simply have a lot of similar lines so it is easy to miss interesting points.

Exactly. It is easy to miss the interesting points. Corollary: it is hard to review. Corollary: it is hard to ensure that no bug was introduced.

The solution is easy, and I have mentioned it several times: split the commit into two, logically-separate patches. One to refactor, one to fix. That would be easy.

I am not a cURL maintainer, but if I were, I would not accept the PR before that split.

return Curl_ssl->get_sizeof_backend_data();
}

static int Curl_multissl_init(void)
{
if(multissl_init(NULL))
@@ -1134,7 +1141,7 @@ static const struct Curl_ssl Curl_ssl_multi = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

(size_t)-1, /* something insanely large to be on the safe side */
Curl_multissl_get_backend_size, /* get_sizeof_backend_data */

Curl_multissl_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -40,7 +40,7 @@ struct Curl_ssl {

unsigned support_https_proxy:1; /* supports access via HTTPS proxies */

size_t sizeof_ssl_backend_data;
size_t (*get_sizeof_backend_data)(void);

int (*init)(void);
void (*cleanup)(void);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.