| Summary: | NoSuchMethodError and JVM crash when running HTTPS test with Tomcat 7 (o.a.t.util.net.TestSsl) | ||
|---|---|---|---|
| Product: | Tomcat Native | Reporter: | Konstantin Kolinko <knst.kolinko> |
| Component: | Library | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | P2 | ||
| Version: | 1.2.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | All | ||
| Attachments: |
Stdout/Stderr output of Apache Ant
JVM crash log |
||
|
Description
Konstantin Kolinko
2015-10-31 01:47:07 UTC
Created attachment 33235 [details] Stdout/Stderr output of Apache Ant Ant output. Testing Tomcat 7 trunk (at r1711498) with JDK 8u66 Created attachment 33236 [details]
JVM crash log
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. I have a fix for this that enables 1.2.x to work with current 7.0.x trunk. I'll be committing that fix shortly. Fixed in trunk for 1.2.1 (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. Review comments all addressed. |