SpotBugs: fix the 227 findings (null dereference, stale thread writes, default charset, exposed internal arrays, constructors that throw) #116

SpotBugs (spotbugs-maven-plugin 4.10.4.1 locally on commit 255b812, default effort/threshold; CI uses 4.9.3.0 from oss-parent, which crashes on JDK 25 class files but reports the same detectors on JDK 17) reports 227 bugs: 13 × priority 1, 214 × priority 2. By category: MALICIOUS_CODE 132, BAD_PRACTICE 41, MT_CORRECTNESS 17, CORRECTNESS 14, STYLE 11, I18N 10, EXPERIMENTAL 1, SECURITY 1. The report is generated by the site build but never gated.

Correctness (fix first)

• NP_GUARANTEED_DEREF – coding/protocol/decoder/ProtocolDecoder.java:195: payload can be null and is dereferenced in decodePayload (an empty IPMI payload from the BMC → NPE → ErrorAction → timeout; see Malformed-packet robustness: unchecked array copies and unbounded pad loops in the decoders #92).
• NP_NULL_ON_SOME_PATH_EXCEPTION ×13 – all in coding/DecoderRunner.java (run()), the main() harness slated for deletion in Dead code and leftovers in core: UdpNotifier, SessionUpkeep, DecoderRunner main() in src/main, unused queue methods, dead UdpMessenger bufferSize #99.
• BC_VACUOUS_INSTANCEOF – coding/commands/IpmiCommandCoder.java:69: instanceof in isCommandResponse is always true.
• UC_USELESS_CONDITION – coding/commands/fru/record/ChassisInfo.java:96: partDataLength != 0 already known.

Multithreaded correctness (17) – overlaps #96, #95, #93

• AT_STALE_THREAD_WRITE_OF_PRIMITIVE ×13: Connection.java:120 (timeout), :216/:391 (sessionId), :453 (managedSystemSessionId); MessageQueue.java:71 (timeout); UdpMessenger.java:107 (bufferSize), :168 (closing); 6 × DecoderRunner. These are the non-volatile fields polled across the caller/UDP/timer threads described in Data races: handshake/state fields polled without volatile, listener lists iterated unsynchronized #96.
• IS2_INCONSISTENT_SYNC – api/sync/MessageListener.java:89: tag locked 50 % of the time (the retry bug Sync retry never waits for the resent message: MessageListener.response is never reset #78 lives in the same method).
• JLM_JSR166_UTILCONCURRENT_MONITORENTER – connection/ConnectionManager.java:131: synchronized on an AtomicInteger (generateSessionlessTag, see ConnectionManager / SessionManager lifecycle bugs: static reservedTags reassigned, inconsistent handles, connections never removed, establishSession tears down the whole connector, keep-alive responses filtered by class #95).
• USO_UNSAFE_STATIC_METHOD_SYNCHRONIZATION – connection/SessionManager.java:52: static synchronized generateSessionId() uses the class lock.
• SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA – transport/UdpMessenger.java:225: static sentPackets modified under an instance lock.
• SWL_SLEEP_WITH_LOCK_HELD – transport/UdpMessenger.java:221: Thread.sleep() inside synchronized send() (the 1 ms-per-packet serialisation noted in Dead code and leftovers in core: UdpNotifier, SessionUpkeep, DecoderRunner main() in src/main, unused queue methods, dead UdpMessenger bufferSize #99).

Bad practice / I18N (41 + 10)

• DM_DEFAULT_ENCODING ×10: Rakp1.java:252/376/402/408/435, Rakp3.java:233, security/AuthenticationAlgorithm.java:102 (credentials and BMC key through the platform charset – this is Credentials handling: BMC key / password / username go through String and the platform charset; char[] password converted to String in the client #90), api/sol/SerialOverLan.java:376/499, fru/record/ManagementAccessInfo.java:57.
• DE_MIGHT_IGNORE – client/runner/GetFrusRunner.java:225 (same empty catch as the PMD issue).
• OBL_UNSATISFIED_OBLIGATION – common/PropertiesManager.java:58: InputStream not closed (PropertiesManager and connection.properties issues: dead cleaningFrequency, NPE on missing resource, unsynchronized lazy init, INFO log per lookup #98).
• ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD ×8: transport/UdpMessenger.java:94 (sentPackets) and 7 × DecoderRunner.
• MS_PKGPROTECT ×2: security/IntegrityAlgorithm.CONST1, security/ConfidentialityAesCbc128.CONST2 (mutable protected static final byte[]).
• MS_EXPOSE_REP – common/PropertiesManager.getInstance().
• URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD – security/ConfidentialityAlgorithm.sik never read.
• CT_CONSTRUCTOR_THROW ×40 (STYLE, "constructor throws → finalizer attack"): ConnectionManager ×4, IpmiAsyncConnector ×3, IpmiConnector ×3, SerialOverLan ×3, GetChannelAuthenticationCapabilities ×3, PayloadCoder, ReadFruData, GetChannelPayloadSupport, IpmiLanRequest, UdpMessenger ×2 each, and 14 more classes ×1. Mostly validation IllegalArgumentExceptions in constructors; make the classes final or move validation to factories where it is cheap, otherwise suppress with a documented @SuppressFBWarnings.

Malicious code / exposure (132)

• EI_EXPOSE_REP ×54 and EI_EXPOSE_REP2 ×75: getters returning / constructors storing mutable byte[] or List without copying, across 60 classes – e.g. IpmiClientConfiguration.getPassword()/getBmcKey() (×6 with the setters), Fru, Sensor, BoardInfo, ProductInfo, Rakp1, Rakp3 ×4, IpmiMessage, IpmiPayload, RmcpMessage, UdpMessage, QueueElement, StateMachine, the *ResponseData classes, the SOL classes. For the protocol-internal classes a defensive copy per packet is unnecessary overhead; decide per package: copy in the public client API (IpmiClientConfiguration, Fru, Sensor, FruRecord subclasses, *ResponseData), and suppress with @SuppressFBWarnings(justification = ...) in the internal coding/transport classes.

Acceptance

• mvn spotbugs:check (or the site report) reports 0 bugs at the default threshold, with explicit, justified @SuppressFBWarnings for the intentional cases.
• Pin spotbugs-maven-plugin ≥ 4.10.4.1 (reads class files of recent JDKs) in <build> and <reporting> at the same version, and add spotbugs:check to verify, as done in winrm-java.
• spotbugs-annotations added as a provided/optional dependency for the suppressions.