https://bz.apache.org/bugzilla/show_bug.cgi?id=58566
Konstantin Kolinko <knst.koli...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|FIXED |---
Status|RESOLVED |REOPENED
--- Comment #5 from Konstantin Kolinko <knst.koli...@gmail.com> ---
(In reply to Mark Thomas from comment #3)
> I am not concerned about the potential race condition here.
>
> In Tomcat, starting of multiple connectors is always single threaded.
>
> In theory, connectors could be started in parallel via the embedded API but
> even then the likelihood of a race is low and, as it happens, the fix for
> the method not being present should prevent any crashes if the race occurs.
There is also JMX API, StandardService.addConnector(...). There is no
indication in java code that SSLContext.make() should be called by a single
thread only (e.g. method not marked as synchronized).
There is a method that is certainly called only once - the method called by
APRLifecycleListener, SSL.initialize(). I wonder whether this code can be moved
there.
I agree that this is not a stopper. Usually connectors have <Connector
bindOnInit="true"/> so there is a delay between initialization of all
connectors and their start. The worst case - serving several requests without
SNI - won't happen because all connectors will be initialized before starting
them.
BTW, in sslcontext.c / SSLContext.make():
/* Cache the byte[].class for performance reasons */
clazz = (*e)->FindClass(e, "[B");
byteArrayClass = (jclass) (*e)->NewGlobalRef(e, clazz);
The same lookup/caching of "[B" is present in ssl.c / SSL.initialize().
Review of r1711667:
(Comment typo already fixed by r1711675)
1. sslcontext.c / SSLContext.make(..)
> /* Older Tomcat versions may not have this static method */
> if ( JNI_TRUE == (*e)->ExceptionCheck(e) ) {
> (*e)->ExceptionClear(e);
> }
Other places (info.c, misc.c) just call
if ((*e)->ExceptionCheck(e)) {
2. sslcontext.c / ssl_callback_ServerNameIndication()
> if (sni_java_callback != 0) {
1) sni_java_callback is jmethodID which is a pointer to a structure. So != NULL
2) If callback call is skipped, new_ssl_context variable is left uninitialized
(random value) and "if (original_ssl_context != new_ssl_context) {" check is
satisfied and goes on accessing random memory.
3) If sni_java_callback is unavailable, this method should return early. It
does a lot of unneeded work.
4) Java method SSLContext.sniCallBack() implemented in Tomcat trunk by default
returns 0
As such, "if (original_ssl_context != new_ssl_context) {" compares not-null
pointer with 0 returned by the method.
5)
> SSL_set_SSL_CTX(ssl, J2P(new_ssl_context, SSL_CTX *));
Is "new_ssl_context" here a pointer to SSL_CTX ?
I cannot confirm it. From java code it looks that it is a pointer to
tcn_ssl_ctxt_t structure, which field "ctx" is (SSL_CTX*).
Looking at Tomcat trunk code, the method
in APREndpoint.bind():
> sslHostConfig.setOpenSslContext(Long.valueOf(ctx));
ctx is a pointer to tcn_ssl_ctxt_t structure, not a SSL_CTX pointer.
6) In SSLHostConfig.java in Tomcat trunk:
public Object getOpenSslContext() {
return openSslContext;
}
s/Object/Long/, to match setter method and to avoid class casts later.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
