Add SpotBugs static analysis, fix bugs found#204
Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to integrate SpotBugs static analysis into the build/CI while also making a set of targeted code changes to reduce/avoid common static-analysis findings (constants immutability, safer deserialization, stricter parsing/reads, and a few concurrency-related tweaks) across the wolfCrypt JNI/JCE codebase.
Changes:
- Add SpotBugs support: Ant target, exclusion filter, and a GitHub Actions workflow to run it on PRs.
- Harden a few security/robustness paths (e.g.,
readFully()for keystore decoding, DER length validation, explicit exceptions for unsupported enum/switch values). - Make various public “constant” fields
finaland adjust some thread-safety/serialization behaviors.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/wolfssl/wolfcrypt/WolfSSLCertManager.java | Adjust finalize() visibility. |
| src/main/java/com/wolfssl/wolfcrypt/WolfObject.java | Clarify static-init/FIPS CAST comment. |
| src/main/java/com/wolfssl/wolfcrypt/WolfCrypt.java | Make various exported values static final. |
| src/main/java/com/wolfssl/wolfcrypt/Sha3.java | Add stricter hash type handling and volatile fields. |
| src/main/java/com/wolfssl/wolfcrypt/Rng.java | Make max block len static final. |
| src/main/java/com/wolfssl/provider/jce/WolfSSLKeyStore.java | Improve RNG reuse notes, safer stream handling, and use readFully() for decoding. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptRandom.java | Adjust readObject synchronization. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptPKIXRevocationChecker.java | Mark initialized as volatile. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptPKIXCertPathValidator.java | Throw when trust anchor can’t be found instead of skipping constraints check. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java | Restore transient lock on deserialization via readObject. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptKeyPairGenerator.java | Simplify DH init logging. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptKeyGenerator.java | Add default case guarding unsupported AlgoType. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptDhParameters.java | Validate DER sequence length against input. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptDhParameterGenerator.java | Remove stored SecureRandom field; rely on native RNG during generation. |
| src/main/java/com/wolfssl/provider/jce/WolfCryptDebug.java | Simplify logging record creation. |
| spotbugs-exclude.xml | Add SpotBugs exclusion filter for reviewed/intentional findings. |
| jni/include/com_wolfssl_wolfcrypt_WolfCrypt.h | Regenerate/update JNI header constants for newly-final fields. |
| build.xml | Add ant spotbugs target and checks. |
| .github/workflows/spotbugs.yml | Add CI job to run SpotBugs and fail on warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/wolfssl/provider/jce/WolfCryptDhParameterGenerator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR primarily tightens up code quality and static-analysis hygiene across the wolfCrypt JNI/JCE codebase, and adds CI/build support for SpotBugs while also addressing a handful of correctness and concurrency concerns in the Java provider code.
Changes:
- Add SpotBugs support (Ant target, GitHub Actions workflow) and introduce a repository SpotBugs exclusion filter.
- Improve robustness of keystore/key parsing and PKIX validation (e.g.,
readFully(), trust-anchor handling). - Apply targeted thread-safety / API-hardening tweaks (e.g.,
finalconstants,volatilestate, safer deserialization).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/com/wolfssl/wolfcrypt/WolfSSLCertManager.java |
Adjust finalizer visibility to properly override Object.finalize(). |
src/main/java/com/wolfssl/wolfcrypt/WolfObject.java |
Clarify static-init comment around FIPS CAST / subclass reference. |
src/main/java/com/wolfssl/wolfcrypt/WolfCrypt.java |
Make various public mapping fields final; keep enum/define mappings consistent. |
src/main/java/com/wolfssl/wolfcrypt/Sha3.java |
Add volatile to key fields; add defensive default in digest-size switch. |
src/main/java/com/wolfssl/wolfcrypt/Rng.java |
Make RNG max-block-length constant final. |
src/main/java/com/wolfssl/provider/jce/WolfSSLKeyStore.java |
Improve keystore entry decoding with readFully(), safer close, reduce RNG allocations, make inner stream class static. |
src/main/java/com/wolfssl/provider/jce/WolfCryptRandom.java |
Adjust deserialization helper signature (remove synchronized). |
src/main/java/com/wolfssl/provider/jce/WolfCryptPKIXRevocationChecker.java |
Mark initialized as volatile for cross-thread visibility. |
src/main/java/com/wolfssl/provider/jce/WolfCryptPKIXCertPathValidator.java |
Enforce non-null trust anchor and throw a clear CertPathValidatorException otherwise. |
src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java |
Restore transient lock on deserialization via readObject. |
src/main/java/com/wolfssl/provider/jce/WolfCryptKeyPairGenerator.java |
Simplify DH init logging. |
src/main/java/com/wolfssl/provider/jce/WolfCryptKeyGenerator.java |
Add defensive default case in AlgoType switch. |
src/main/java/com/wolfssl/provider/jce/WolfCryptDhParameters.java |
Add SEQUENCE length validation when parsing DER DH params. |
src/main/java/com/wolfssl/provider/jce/WolfCryptDhParameterGenerator.java |
Document/implement ignoring caller SecureRandom and add retry behavior for prime generation failures. |
src/main/java/com/wolfssl/provider/jce/WolfCryptDebug.java |
Change how log “source class” is set in LogRecord. |
spotbugs-exclude.xml |
New SpotBugs exclusions filter for reviewed false positives/intentional patterns. |
build.xml |
Add ant spotbugs target with environment + Java-version checks. |
.github/workflows/spotbugs.yml |
New PR workflow to run SpotBugs and fail on warnings. |
.github/workflows/android_gradle.yml |
Pin emulator-runner action version and adjust emulator boot/animation handling. |
jni/include/com_wolfssl_wolfcrypt_WolfCrypt.h |
Regenerated JNI header constants for SSL/TLS define mappings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…h in WolfCryptDhParameters
… remove unused field, make inner class static
Summary
ant spotbugstarget andspotbugs-exclude.xmlexclusion filterBug fixes found by SpotBugs
read()->readFully())destroyedLocknot restored)Code improvements
finalto public static int constants in WolfCrypt and Rngvolatileto shared fields in Sha3 and WolfCryptPKIXRevocationCheckerSpotBugs exclusions
Reviewed and excluded intentional design patterns including JNI constructor feature-detect guards, JCE naming conventions, clone() without super.clone() (native pointer safety), finalizer null-after-release patterns, and internal state sharing. All exclusions scoped to specific classes, methods, and fields