Skip to content

Commit fc867c3

Browse files
cconloncconlon
committed
JNI/JSSE: Fix threading issues found by SpotBugs
- Fix inconsistent synchronization in WolfSSLParameters for getCipherSuites/setCipherSuites and getApplicationProtocols/setApplicationProtocols - Add volatile to WolfSSLImplementSSLSession.isInTable and alpnSelector fields - Use class lock for static fields in detectSecurityPropertySettings() - Add synchronized to WolfSSLEngine.getEnabledCipherSuites()
1 parent bd8606d commit fc867c3

6 files changed

Lines changed: 19 additions & 158 deletions

File tree

‎spotbugs-exclude.xml‎

Lines changed: 0 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -299,153 +299,4 @@
299299
<Bug pattern="IS2_INCONSISTENT_SYNC"/>
300300
</Match>
301301

302-
<!--
303-
NM_METHOD_NAMING_CONVENTION: Public native JNI methods use
304-
PascalCase to match native wolfSSL C function naming.
305-
Renaming would break existing users and require updating
306-
native JNI C code.
307-
-->
308-
<Match>
309-
<Class name="com.wolfssl.WolfSSL"/>
310-
<Bug pattern="NM_METHOD_NAMING_CONVENTION"/>
311-
</Match>
312-
313-
<!--
314-
MS_PKGPROTECT: Public API constants in WolfSSL documented
315-
in Javadoc as values users pass to methods like
316-
getExtensionSet(), setKeyUsage(), setPublicKey(), etc.
317-
Reducing visibility would break users.
318-
-->
319-
<Match>
320-
<Class name="com.wolfssl.WolfSSL"/>
321-
<Field name="NID_surname"/>
322-
<Bug pattern="MS_PKGPROTECT"/>
323-
</Match>
324-
<Match>
325-
<Class name="com.wolfssl.WolfSSL"/>
326-
<Field name="NID_serialNumber"/>
327-
<Bug pattern="MS_PKGPROTECT"/>
328-
</Match>
329-
<Match>
330-
<Class name="com.wolfssl.WolfSSL"/>
331-
<Field name="NID_pkcs9_unstructuredName"/>
332-
<Bug pattern="MS_PKGPROTECT"/>
333-
</Match>
334-
<Match>
335-
<Class name="com.wolfssl.WolfSSL"/>
336-
<Field name="NID_pkcs9_contentType"/>
337-
<Bug pattern="MS_PKGPROTECT"/>
338-
</Match>
339-
<Match>
340-
<Class name="com.wolfssl.WolfSSL"/>
341-
<Field name="NID_pkcs9_challengePassword"/>
342-
<Bug pattern="MS_PKGPROTECT"/>
343-
</Match>
344-
<Match>
345-
<Class name="com.wolfssl.WolfSSL"/>
346-
<Field name="NID_givenName"/>
347-
<Bug pattern="MS_PKGPROTECT"/>
348-
</Match>
349-
<Match>
350-
<Class name="com.wolfssl.WolfSSL"/>
351-
<Field name="NID_initials"/>
352-
<Bug pattern="MS_PKGPROTECT"/>
353-
</Match>
354-
<Match>
355-
<Class name="com.wolfssl.WolfSSL"/>
356-
<Field name="NID_key_usage"/>
357-
<Bug pattern="MS_PKGPROTECT"/>
358-
</Match>
359-
<Match>
360-
<Class name="com.wolfssl.WolfSSL"/>
361-
<Field name="NID_subject_alt_name"/>
362-
<Bug pattern="MS_PKGPROTECT"/>
363-
</Match>
364-
<Match>
365-
<Class name="com.wolfssl.WolfSSL"/>
366-
<Field name="NID_basic_constraints"/>
367-
<Bug pattern="MS_PKGPROTECT"/>
368-
</Match>
369-
<Match>
370-
<Class name="com.wolfssl.WolfSSL"/>
371-
<Field name="NID_ext_key_usage"/>
372-
<Bug pattern="MS_PKGPROTECT"/>
373-
</Match>
374-
<Match>
375-
<Class name="com.wolfssl.WolfSSL"/>
376-
<Field name="NID_dnQualifier"/>
377-
<Bug pattern="MS_PKGPROTECT"/>
378-
</Match>
379-
<Match>
380-
<Class name="com.wolfssl.WolfSSL"/>
381-
<Field name="RSAk"/>
382-
<Bug pattern="MS_PKGPROTECT"/>
383-
</Match>
384-
<Match>
385-
<Class name="com.wolfssl.WolfSSL"/>
386-
<Field name="ECDSAk"/>
387-
<Bug pattern="MS_PKGPROTECT"/>
388-
</Match>
389-
390-
<!--
391-
FI_FINALIZER_NULLS_FIELDS: Finalizers null out fields as
392-
defensive cleanup for JNI native resources. Nulling ensures
393-
references to native pointers and shared objects don't
394-
linger if something holds a reference to the finalized
395-
object. The finalizers also do real work (freeSSL(),
396-
store.clear()). Harmless and intentional.
397-
-->
398-
<Match>
399-
<Class name="com.wolfssl.provider.jsse.WolfSSLAuthStore"/>
400-
<Method name="finalize"/>
401-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
402-
</Match>
403-
<Match>
404-
<Class name="com.wolfssl.provider.jsse.WolfSSLContext"/>
405-
<Method name="finalize"/>
406-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
407-
</Match>
408-
<Match>
409-
<Class name="com.wolfssl.provider.jsse.WolfSSLEngine"/>
410-
<Method name="finalize"/>
411-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
412-
</Match>
413-
<Match>
414-
<Class name=
415-
"com.wolfssl.provider.jsse.WolfSSLEngineHelper"/>
416-
<Method name="finalize"/>
417-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
418-
</Match>
419-
<Match>
420-
<Class name="com.wolfssl.provider.jsse.WolfSSLSocket"/>
421-
<Method name="finalize"/>
422-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
423-
</Match>
424-
<Match>
425-
<Class name="com.wolfssl.provider.jsse.WolfSSLTrustX509"/>
426-
<Method name="finalize"/>
427-
<Bug pattern="FI_FINALIZER_NULLS_FIELDS"/>
428-
</Match>
429-
430-
<!--
431-
PA_PUBLIC_PRIMITIVE_ATTRIBUTE: WolfSSLDebug public debug
432-
flags are public API for users to check/control debug
433-
logging at runtime.
434-
-->
435-
<Match>
436-
<Class name="com.wolfssl.WolfSSLDebug"/>
437-
<Field name="DEBUG"/>
438-
<Bug pattern="PA_PUBLIC_PRIMITIVE_ATTRIBUTE"/>
439-
</Match>
440-
<Match>
441-
<Class name="com.wolfssl.WolfSSLDebug"/>
442-
<Field name="DEBUG_JNI"/>
443-
<Bug pattern="PA_PUBLIC_PRIMITIVE_ATTRIBUTE"/>
444-
</Match>
445-
<Match>
446-
<Class name="com.wolfssl.WolfSSLDebug"/>
447-
<Field name="DEBUG_JSON"/>
448-
<Bug pattern="PA_PUBLIC_PRIMITIVE_ATTRIBUTE"/>
449-
</Match>
450-
451302
</FindBugsFilter>

‎src/java/com/wolfssl/WolfSSLSession.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,11 @@ private void detectSecurityPropertySettings() {
237237
if (readWritePoolDisabled()) {
238238
this.byteBufferPoolEnabled = false;
239239
}
240+
}
240241

242+
/* Use class lock for static fields shared across all
243+
* WolfSSLSession instances */
244+
synchronized (WolfSSLSession.class) {
241245
/* Check if the maximum size of the static per-thread direct
242246
* ByteBuffer pool has been adjusted by setting of the
243247
* "wolfssl.readWriteByteBufferPool.size" Security property. */

‎src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ public class WolfSSLEngine extends SSLEngine {
164164
private final Object toSendLock = new Object();
165165

166166
/** ALPN selector callback, if set */
167-
protected BiFunction<SSLEngine, List<String>, String> alpnSelector = null;
167+
protected volatile
168+
BiFunction<SSLEngine, List<String>, String> alpnSelector = null;
168169

169170
/** Turn on extra/verbose SSLEngine debug logging */
170171
private boolean extraDebugEnabled = false;
@@ -2069,7 +2070,7 @@ public String[] getSupportedCipherSuites() {
20692070
}
20702071

20712072
@Override
2072-
public String[] getEnabledCipherSuites() {
2073+
public synchronized String[] getEnabledCipherSuites() {
20732074
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
20742075
() -> "entered getEnabledCipherSuites()");
20752076
return this.engineHelper.getCiphers();

‎src/java/com/wolfssl/provider/jsse/WolfSSLImplementSSLSession.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public class WolfSSLImplementSSLSession extends ExtendedSSLSession {
9292
* true and the sesPtr is then freed by that object either during
9393
* setResume() or finalization.
9494
*/
95-
protected boolean isInTable = false;
95+
protected volatile boolean isInTable = false;
9696

9797
/**
9898
* Tracks if WOLFSSL_SESSION pointer has been updated after retreived from

‎src/java/com/wolfssl/provider/jsse/WolfSSLParameters.java‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ public WolfSSLParameters() {
8888
}
8989
}
9090

91-
/* create duplicate copy of these parameters */
91+
/**
92+
* Create duplicate copy of these parameters.
93+
*
94+
* @return new WolfSSLParameters copy of this object
95+
*/
9296
protected synchronized WolfSSLParameters copy() {
9397
WolfSSLParameters cp = new WolfSSLParameters();
9498
cp.setCipherSuites(this.cipherSuites);
@@ -127,15 +131,15 @@ protected synchronized WolfSSLParameters copy() {
127131
}
128132

129133
@Override
130-
public String[] getCipherSuites() {
134+
public synchronized String[] getCipherSuites() {
131135
if (this.cipherSuites == null) {
132136
return null;
133137
}
134138
return this.cipherSuites.clone();
135139
}
136140

137141
@Override
138-
public void setCipherSuites(String[] cipherSuites) {
142+
public synchronized void setCipherSuites(String[] cipherSuites) {
139143
/* cipherSuites array is sanitized by wolfJSSE caller */
140144
if (cipherSuites == null) {
141145
this.cipherSuites = null;
@@ -271,14 +275,14 @@ byte[] getAlpnProtos() {
271275
* methods, which may not exist on older runtimes.
272276
*/
273277

274-
public String[] getApplicationProtocols() {
278+
public synchronized String[] getApplicationProtocols() {
275279
if (this.applicationProtocols == null) {
276280
return null;
277281
}
278282
return this.applicationProtocols.clone();
279283
}
280284

281-
public void setApplicationProtocols(String[] protocols) {
285+
public synchronized void setApplicationProtocols(String[] protocols) {
282286
if (protocols == null) {
283287
this.applicationProtocols = new String[0];
284288
}

‎src/java/com/wolfssl/provider/jsse/WolfSSLSocket.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ public class WolfSSLSocket extends SSLSocket {
114114
private final Object initLock = new Object();
115115

116116
/** ALPN selector callback, if set */
117-
protected BiFunction<SSLSocket, List<String>, String> alpnSelector = null;
117+
protected volatile
118+
BiFunction<SSLSocket, List<String>, String> alpnSelector = null;
118119

119120
/* true if client, otherwise false */
120121
private boolean isClientMode = false;

0 commit comments

Comments
 (0)