-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
Remove dependency on apr by using c++11 / c++14 features #255
Conversation
not 100 % done yet. Will ask for review once I am done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer looks great! Just a few nits
openssl-dynamic/src/main/c/ssl.c
Outdated
CRYPTO_set_dynlock_create_callback(ssl_dyn_create_function); | ||
CRYPTO_set_dynlock_lock_callback(ssl_dyn_lock_function); | ||
CRYPTO_set_dynlock_destroy_callback(ssl_dyn_destroy_function); | ||
|
||
apr_pool_cleanup_register(p, NULL, ssl_thread_cleanup, | ||
apr_pool_cleanup_null); | ||
//apr_pool_cleanup_register(p, NULL, ssl_thread_cleanup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for a good way to call these again. So stay tuned
openssl-dynamic/src/main/c/ssl.c
Outdated
apr_pool_cleanup_register(tcn_global_pool, NULL, | ||
ssl_init_cleanup, | ||
apr_pool_cleanup_null); | ||
//apr_pool_cleanup_register(tcn_global_pool, NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for a good way to call these again. So stay tuned
#include "ssl_private.h" | ||
#include <string.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Was it previously brought in by another header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep was brought it by another header before.
be9aace
to
105f955
Compare
@nmittler @Scottmitch PTAL... it compiles here and on my linux / windows vm. Need to update the g++ version on the ci. |
d242b52
to
946520b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Goodbye APR! :)
@trustin PTAL as well. |
openssl-dynamic/src/main/c/ssl.c
Outdated
@@ -1987,3 +1918,8 @@ TCN_IMPLEMENT_CALL(void, SSL, freeX509Chain)(TCN_STDARGS, jlong x509Chain) | |||
sk_X509_pop_free(chain, X509_free); | |||
} | |||
|
|||
void tcn_ssl_unload() { | |||
if (ssl_init_cleanup()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just have a single method which does both cleanups? the ssl_init_cleanup
method is also called in the initialize
above, but initialize
also calls ssl_thread_setup
so it seems like we should cleanup both anyways. It also is awkward to split the cleanup amongst 2 methods in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure can do
@@ -212,13 +213,13 @@ struct tcn_ssl_ctxt_t { | |||
/* TLS ticket key session resumption statistics */ | |||
|
|||
// The client did not present a ticket and we issued a new one. | |||
apr_uint32_t ticket_keys_new; | |||
tcn_atomic_uint32_t ticket_keys_new; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz fix the alignment for the variable names so they line up with the other variable names like before this change.
} | ||
|
||
void tcn_atomic_uint32_free(tcn_atomic_uint32_t atomic) { | ||
auto *p = (std::atomic<uint32_t> *) atomic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider removing the temp variable.
also consider passing a **
as a parameter so you can always set *atomic = NULL;
in these methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment applies to all the cpp
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
|
||
if ((c = apr_pcalloc(p, sizeof(tcn_ssl_ctxt_t))) == NULL) { | ||
tcn_ThrowAPRException(e, apr_get_os_error()); | ||
if ((c = OPENSSL_malloc(sizeof(tcn_ssl_ctxt_t))) == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider just using calloc
instead of OPENSSL_malloc
. IIUC OpenSSL is not aware of this variable and therefore we don't need to use OpenSSL allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also make sure you use free
instead of OPENSSL_free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good... this way we can also remove the memset
return ssl_context_cleanup(c); | ||
ssl_context_cleanup(c); | ||
|
||
// TODO: Fix me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we fix this now ... just make this function void?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like said before I wanted to do the pr without api breakage to make it easier to consume for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a followup PR is fine but the return value is not used in Netty. I don't think this will break anything.
@@ -29,12 +29,14 @@ AC_CANONICAL_HOST | |||
AC_CANONICAL_SYSTEM | |||
|
|||
${CFLAGS="-O3 -Werror -fno-omit-frame-pointer -Wunused-variable"} | |||
${CXXFLAGS="-O3 -Werror -fno-omit-frame-pointer -Wunused-variable"} | |||
# Ensure we support c++11 for atomics etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c++11
-> c++14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
extern "C" { | ||
#endif // __cplusplus | ||
|
||
typedef void* tcn_lock_rw_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have lock_rw
, lock_reader
, and lock_writer
. Consider being consistent/complete with the naming:
lock_reader_writer
,lock_reader
, andlock_writer
lock_rw
,lock_r
, andlock_w
Also consider making the file name consistent if you go with the more verbose names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
openssl-dynamic/src/main/c/ssl.c
Outdated
} | ||
|
||
value = (struct CRYPTO_dynlock_value *)apr_palloc(p, | ||
value = (struct CRYPTO_dynlock_value *)OPENSSL_malloc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider declaring and initializing this variable in the same statement.
openssl-dynamic/src/main/c/ssl.c
Outdated
{ | ||
UNREFERENCED(data); | ||
CRYPTO_set_locking_callback(NULL); | ||
CRYPTO_THREADID_set_callback(NULL); | ||
CRYPTO_set_dynlock_create_callback(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't look like this method takes ownership of the memory [1]. We should take more care to free this memory, and if we set it check if there already exists something and free it.
[1] https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/crypto/cryptlib.c#L376
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also be sure to use the ssl_dyn_destroy_function
(should be accessible via CRYPTO_get_dynlock_destroy_callback
) on the result of CRYPTO_get_dynlock_create_callback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm why would this be needed if we not malloc the callbacks before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify? previously a apr_palloc
was used to allocate the CRYPTO_dynlock_value
structure. This PR uses OPENSSL_malloc
instead of the apr_palloc
method. Either way there is dynamic memory being allocated. I'm not sure where/if it was freed before, but unless you find that OpenSSL is taking ownership in this situation (just setting the variable to NULL
) then we need to ensure this dynamic memory is cleaned up before we lose the reference to it.
openssl-dynamic/src/main/c/ssl.c
Outdated
if (rv != APR_SUCCESS) { | ||
/* TODO log that fprintf(stderr, "Failed to destroy mutex for dynamic lock %s:%d", l->file, l->line); */ | ||
} | ||
free((void *) l->file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the cast necessary? can we just make the type char*
in the structure definition if the const
is confusing things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me see if this works.
Doh brain fart ... you are right let me fix it
… Am 28.03.2017 um 19:52 schrieb Scott Mitchell ***@***.***>:
@Scottmitch commented on this pull request.
In openssl-dynamic/src/main/c/ssl.c:
> {
- UNREFERENCED(data);
CRYPTO_set_locking_callback(NULL);
CRYPTO_THREADID_set_callback(NULL);
CRYPTO_set_dynlock_create_callback(NULL);
can you clarify? previously a apr_palloc was used to allocate the CRYPTO_dynlock_value structure. This PR uses OPENSSL_malloc instead of the apr_palloc method. Either way there is dynamic memory being allocated. I'm not sure where/if it was freed before, but unless you find that OpenSSL is taking ownership in this situation (just setting the variable to NULL) then we need to ensure this dynamic memory is cleaned up before we lose the reference to it.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Good point. Let me change it
… Am 28.03.2017 um 19:55 schrieb Scott Mitchell ***@***.***>:
@Scottmitch commented on this pull request.
In openssl-dynamic/src/main/c/sslcontext.c:
> @@ -317,7 +328,10 @@ TCN_IMPLEMENT_CALL(jint, SSLContext, free)(TCN_STDARGS, jlong ctx)
UNREFERENCED_STDARGS;
TCN_ASSERT(ctx != 0);
- return ssl_context_cleanup(c);
+ ssl_context_cleanup(c);
+
+ // TODO: Fix me
Doing a followup PR is fine but the return value is not used in Netty. I don't think this will break anything.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
946520b
to
06c4109
Compare
openssl-dynamic/src/main/c/ssl.c
Outdated
/* TODO log that fprintf(stderr, "Failed to create subpool for dynamic lock"); */ | ||
return NULL; | ||
} | ||
value = (struct CRYPTO_dynlock_value *)malloc(sizeof(struct CRYPTO_dynlock_value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider declaring and initializing this variable in the same statement.
|
||
CRYPTO_set_locking_callback(NULL); | ||
CRYPTO_THREADID_set_callback(NULL); | ||
CRYPTO_set_dynlock_create_callback(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a link as to where the previous callback is freed if we just set the callback to NULL
? When I looked at OpenSSL src it seemed like this created a memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not yet, will do today (and it was not freed before in tcnative as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think all is good now. Please ping me once you are awake and we can talk about it in more detail if you have doubts. Maybe I am just missing something but from my look into the source etc its just fine now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't track it all the way down but found that there are links to the callbacks retained in the engines ... so ENGINE_cleanup
maybe the place where these callbacks are used to clean stuff up. my concern was we were missing a call which notifies OpenSSL to use these callbacks. @davidben - can you shed some light on this situation?
openssl-dynamic/src/main/c/ssl.c
Outdated
} | ||
#endif | ||
|
||
TCN_IMPLEMENT_CALL(jint, SSL, initialize)(TCN_STDARGS, jstring engine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this can just be void because we throw an exception upon failure anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the method return type was changed in the java header, but not here ... seems like there will be a mismatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... noe done yet.
openssl-dynamic/src/main/c/ssl.c
Outdated
@@ -2113,3 +2043,7 @@ TCN_IMPLEMENT_CALL(jbyteArray, SSL, getOcspResponse)(TCN_STDARGS, jlong ssl) { | |||
return value; | |||
#endif | |||
} | |||
|
|||
void tcn_ssl_unload() { | |||
ssl_init_cleanup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you combine tcn_ssl_unload
and ssl_init_cleanup
? seems unnecessary to have a function which only calls a single function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@@ -87,14 +84,19 @@ static apr_status_t ssl_context_cleanup(void *data) | |||
free(c->password); | |||
c->password = NULL; | |||
} | |||
|
|||
tcn_atomic_uint32_free(&(c->ticket_keys_new)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&(c->ticket_keys_new)
-> &c->ticket_keys_new
? The parens shouldn't be necessary, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
|
||
tcn_lock_rw_t tcn_lock_rw_new() { | ||
// Once we switch to c++17 we should use std::shared_mutex | ||
return (tcn_lock_rw_t) new std::shared_timed_mutex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC in C++ it is ambiguous if you try to use ()
for the no argument constructor without the new
keyword ... new
may make the situation unambiguous but consider dropping the ()
which may be more C++
ish.
|
||
tcn_lock_w_t tcn_lock_w_acquire(tcn_lock_rw_t lock) { | ||
auto *m = (std::shared_timed_mutex *) lock; | ||
return (tcn_lock_w_t) new std::lock_guard<std::shared_timed_mutex>(*m); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not std::unique_lock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also consider removing the temp variable so you don't have to worry about auto
or types:
return (tcn_lock_w_t) new std::lock_guard<std::shared_timed_mutex>(*((std::shared_timed_mutex*) lock));
} | ||
|
||
tcn_lock_r_t tcn_lock_r_acquire(tcn_lock_rw_t lock) { | ||
auto *m = (std::shared_timed_mutex *) lock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider removing the temp variable so you don't have to worry about auto or types:
return (tcn_lock_r_t) new std::shared_lock<std::shared_timed_mutex>(*((std::shared_timed_mutex*) lock));
#ifndef TCN_THREAD_H | ||
#define TCN_THREAD_H | ||
|
||
#include <stddef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to be sure you include the cpp headers if you are in cpp, and the c headers if you are in c. They may define types differently or have different behaviors.
#ifdef __cplusplus
#include <cstddef>
extern "C" {
#else
#include <stddef.h>
#endif // __cplusplus
#ifndef TCN_ATOMIC_H | ||
#define TCN_ATOMIC_H | ||
|
||
#include <stdint.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to be sure you include the cpp headers if you are in cpp, and the c headers if you are in c. They may define types differently or have different behaviors.
#ifdef __cplusplus
#include <cstdint>
extern "C" {
#else
#include <stdint.h>
#endif // __cplusplus
dd5b0f7
to
8960e1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small cleanup ... and just the memory management question left then lgtm.
openssl-dynamic/src/main/c/ssl.c
Outdated
/* | ||
* Dynamic lock creation callback | ||
*/ | ||
static struct CRYPTO_dynlock_value *ssl_dyn_create_function(const char *file, | ||
int line) | ||
{ | ||
struct CRYPTO_dynlock_value *value; | ||
apr_pool_t *p; | ||
apr_status_t rv; | ||
value = (struct CRYPTO_dynlock_value *)malloc(sizeof(struct CRYPTO_dynlock_value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider declaring and initializing this variable in the same statement.
struct CRYPTO_dynlock_value* value = (struct CRYPTO_dynlock_value*) malloc(sizeof(struct CRYPTO_dynlock_value));
openssl-dynamic/src/main/c/ssl.c
Outdated
@@ -106,7 +99,7 @@ struct TCN_bio_bytebuffer { | |||
* so that the return value for SSL_OP_FUTURE_WORKAROUND will only be | |||
* reported by versions that actually support that specific workaround. | |||
*/ | |||
static const jint supported_ssl_opts = 0 | |||
static const jlong supported_ssl_opts = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets hold off on this change and just rebase when #258 is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
||
CRYPTO_set_locking_callback(NULL); | ||
CRYPTO_THREADID_set_callback(NULL); | ||
CRYPTO_set_dynlock_create_callback(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't track it all the way down but found that there are links to the callbacks retained in the engines ... so ENGINE_cleanup
maybe the place where these callbacks are used to clean stuff up. my concern was we were missing a call which notifies OpenSSL to use these callbacks. @davidben - can you shed some light on this situation?
@@ -74,7 +71,7 @@ static apr_status_t ssl_context_cleanup(void *data) | |||
} | |||
c->alpn_proto_len = 0; | |||
|
|||
apr_thread_rwlock_destroy(c->mutex); | |||
tcn_lock_rw_free(&(c->ticket_keys_lock)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&(c->ticket_keys_lock)
-> &c->ticket_keys_lock
openssl-dynamic/src/main/c/ssl.c
Outdated
@@ -2113,3 +2041,4 @@ TCN_IMPLEMENT_CALL(jbyteArray, SSL, getOcspResponse)(TCN_STDARGS, jlong ssl) { | |||
return value; | |||
#endif | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kill the extra new line
56646cd
to
21c6ffb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way to go!
} | ||
|
||
#if OPENSSL_VERSION_NUMBER < 0x10100000L | ||
if (SSLeay() < 0x0090700L) { | ||
TCN_FREE_CSTRING(engine); | ||
tcn_ThrowAPRException(e, APR_EINVAL); | ||
tcn_ThrowException(e, "openssl version too old"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about appending the version number and the minimum required version number?
@@ -29,12 +29,14 @@ AC_CANONICAL_HOST | |||
AC_CANONICAL_SYSTEM | |||
|
|||
${CFLAGS="-O3 -Werror -fno-omit-frame-pointer -Wunused-variable"} | |||
${CXXFLAGS="-O3 -Werror -fno-omit-frame-pointer -Wunused-variable"} | |||
# Ensure we support c++14 for atomics etc | |||
${CXXFLAGS="-O3 -Werror -fno-omit-frame-pointer -Wunused-variable -std=c++14"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps should we static-link libgcc and libstdc++? -static-libgcc -static-libstdc++
Also, we may eventually want to link against musl or uClibc instead of GLIBC so our binary works on more distros. http://stackoverflow.com/a/26306630
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we not want to do any static linking in our dynamic build. Not sure about the "-static" ones tho. @Scottmitch @nmittler WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the situation has changed, but I had an impression that libstdc++ has worse backward ABI compatibility historically. Maybe I'm completely wrong, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets handle this as a followup PR if it becomes an issue or is known to improve things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few more comments then lgtm
openssl-dynamic/src/main/c/ssl.c
Outdated
|
||
#ifndef OPENSSL_NO_ENGINE | ||
if (J2S(engine)) { | ||
ENGINE *ee = NULL; | ||
apr_status_t err = APR_SUCCESS; | ||
int err = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use bool
?
openssl-dynamic/src/main/c/ssl.c
Outdated
@@ -721,19 +656,11 @@ TCN_IMPLEMENT_CALL(jint, SSL, initialize)(TCN_STDARGS, jstring engine) | |||
if (r) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not introduce by this PR, but while we are here) r
is an integer type ... lets use it like an integer: r != 0
#ifndef TCN_ATOMIC_H | ||
#define TCN_ATOMIC_H | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: kill line
#ifndef TCN_THREAD_H | ||
#define TCN_THREAD_H | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: kill line
@normanmaurer - are you planning on pulling this in? |
Yes just wanted to do a few more tests
… Am 07.05.2017 um 00:31 schrieb Scott Mitchell ***@***.***>:
@normanmaurer - are you planning on pulling this in?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Motivation: We had some limited use of apr left in our tcnative fork which complicated the deployment. This apr usages can be replaced by using c++. Modification: Remove the dependency on apr by using c++11 / c++14 features Result: No apr dependeny
@Scottmitch PTAL again.. rebased on latest master and should are ready to get pulled in. |
After @Scottmitch mentioned to me that centos 6 glibc may not support c++14 I checked in more detail and found out it even not support c++11 . So I guess we will need to life with apr ... @Scottmitch WDYT ? |
ssl_lock_cs = apr_palloc(p, ssl_lock_num_locks * sizeof(*ssl_lock_cs)); | ||
if (!ssl_initialized) | ||
return; | ||
ssl_initialized = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to consider concurrency/visibility for ssl_initialized
? can we just remove ssl_initialized
and consider it a programing error if initialized is called multiple times (can this happen in Netty)?
goto cleanup; | ||
} | ||
|
||
c->protocol = protocol; | ||
c->mode = mode; | ||
c->ctx = ctx; | ||
c->pool = p; | ||
c->ticket_keys = tcn_atomic_uint32_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct? ticket_keys
is of type tcn_ssl_ticket_key_t
but this is assigning it to a new atomic uint32 of type void*
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the intention that this should be ticket_keys_new
and ticket_keys
should be initialized in some other way? It is freed above using OPENSSL_free
.
@normanmaurer - I would like to still remove APR ... we should investigate building a glibc shared library, either publish these instructions or the library, and have folks on older distributions depend upon this instead of apr. Newer distributions which have glibc that support c++14 (eventually every one over time) will require not additional dependency. The statically compiled artifacts may be a bit more tricky due to licensing of glibc. |
How does this affect the ability to build against |
0d59905
to
4937622
Compare
Let me close this for now |
Motivation:
We had some limited use of apr left in our tcnative fork which complicated the deployment. This apr usages can be replaced by using c++.
Modification:
Remove the dependency on apr by using c++11 / c++14 features
Result:
No apr dependeny