Repository navigation
Fix the SpotBugs findings and gate the build on SpotBugs (#116) - #134
Merged
Conversation
SpotBugs 4.10.4 reported 196 bugs on main (DecoderRunner and a few
others were already gone); it now reports 0 and spotbugs:check runs at
verify, pinned to 4.10.4.1 like the site report.
Real fixes:
- ProtocolDecoder.decodePayload: an empty payload threw a
NullPointerException in the payload constructors; it now throws
IllegalArgumentException("Empty payload") (NP_GUARANTEED_DEREF)
- Credentials (part of #90): the user name and password are encoded in
UTF-8 whatever the platform charset, and the BMC key (Kg) is passed to
the HMAC as raw bytes instead of going through new String(key) and
getBytes(), which corrupted any byte of 80h or above into a wrong SIK.
The user name length and its 16-byte limit are counted in encoded
bytes. AuthenticationAlgorithm takes the key and password as byte[]
(DM_DEFAULT_ENCODING). Rakp1Test checks the SIK against IPMI 2.0
section 13.31 and fails on the old code.
- volatile on the fields shared by the caller, UDP and timer threads in
Connection, MessageQueue and UdpMessenger; MessageListener writes its
tag under its lock (AT_STALE_THREAD_WRITE_OF_PRIMITIVE, IS2, part of
#96)
- ConnectionManager locks a dedicated object instead of an AtomicInteger,
SessionManager uses an AtomicInteger instead of a static synchronized
method (JLM, USO)
- UdpMessenger: remove the unused static getSentPackets() debug counter
(ST, SSD); setBufferSize() now sizes the receive buffer, which was
hard-coded to 512 bytes
- SerialOverLan: the overloads documented as using the platform charset
say so with Charset.defaultCharset(); ManagementAccessInfo decodes
ISO-8859-1 like the other FRU 8-bit ASCII fields
- PropertiesManager closes its resource stream (OBL, part of #98)
- Remove a vacuous instanceof (IpmiCommandCoder) and a useless condition
(ChassisInfo); CONST1/CONST2 are private (MS_PKGPROTECT)
Justified suppressions (spotbugs-annotations, provided scope):
- core/package-info.java: CT_CONSTRUCTOR_THROW and EI_EXPOSE_REP (which
also matches EI_EXPOSE_REP2) for the whole protocol core: constructors
validate arguments and guard no security-sensitive state, and response
data and records are mutable holders with public setters, so defensive
copies would protect no invariant
- IpmiClientConfiguration (credentials shared so the caller can wipe
them), Fru and Sensor (result holders), PropertiesManager singleton
Live-verified on the GIGABYTE and Lenovo IMM BMCs: login with cipher
suites 3 and 17 and an encrypted command; a wrong password is still
rejected.
Closes #116
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6d9ddff6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…116) - UdpMessenger.run(): back to the fixed 512-byte receive buffer of main. Sizing it from setBufferSize() raced with the receive thread started by the constructor (the first datagram used the old size); making the setter effective is out of the scope of the SpotBugs clean-up, and the volatile field still fixes the stale write SpotBugs reported - README: the 1.2.03 upgrade summary mentions the UTF-8 credentials, the byte[] AuthenticationAlgorithm methods, the removed UdpMessenger.getSentPackets() and the private CONST1/CONST2 Co-Authored-By: Claude Opus 5.5 <[email protected]>
bertysentry
deleted the
116-spotbugs-fix-the-227-findings-null-dereference-stale-thread-writes-default-charset-exposed-internal-arrays-constructors-that-throw
branch
October 8, 2026 17:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #116.
SpotBugs 4.10.4 reported 196 bugs on
main(the issue counted 227;DecoderRunnerand a few others were already gone). It now reports 0, andspotbugs:checkruns atverify, pinned to 4.10.4.1 in<build>and<reporting>.spotbugs-annotations4.10.4 is aprovideddependency.Real fixes
NP_GUARANTEED_DEREFProtocolDecoder.decodePayload: an empty payload threwNullPointerExceptionin the payload constructors; it now throwsIllegalArgumentException("Empty payload")DM_DEFAULT_ENCODING(RAKP, part of #90)new String(key).getBytes(), which corrupted any byte of80hor above into a wrong SIK. The user name length and its 16-byte limit count encoded bytes.AuthenticationAlgorithmtakes the key and password asbyte[]DM_DEFAULT_ENCODING(others)SerialOverLanoverloads documented as using the platform charset now passCharset.defaultCharset().ManagementAccessInfodecodes ISO-8859-1, like the other FRU 8-bit ASCII fieldsAT_STALE_THREAD_WRITE_OF_PRIMITIVE,IS2_INCONSISTENT_SYNC(part of #96)volatileon the fields shared by the caller, UDP and timer threads inConnection,MessageQueueandUdpMessenger.MessageListenerwrites its tag under its lockJLM_JSR166_UTILCONCURRENT_MONITORENTER,USO_UNSAFE_STATIC_METHOD_SYNCHRONIZATIONConnectionManagerlocks a dedicated object instead of anAtomicInteger.SessionManageruses anAtomicIntegerinstead of a static synchronized methodST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD,SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATAUdpMessenger.getSentPackets().setBufferSize()now sizes the receive buffer, which was hard-coded to 512 bytesOBL_UNSATISFIED_OBLIGATION(part of #98)PropertiesManagercloses its resource streamBC_VACUOUS_INSTANCEOF,UC_USELESS_CONDITION,MS_PKGPROTECTinstanceofinIpmiCommandCoderand the dead condition inChassisInfo.CONST1/CONST2areprivateJustified suppressions
core/package-info.java:CT_CONSTRUCTOR_THROWandEI_EXPOSE_REPfor the whole protocol core. A package-level@SuppressFBWarningsalso covers subpackages, andEI_EXPOSE_REPalso matchesEI_EXPOSE_REP2. Constructors reject invalid arguments and guard no security-sensitive state. Response data and records are mutable holders with public setters, so defensive copies would protect no invariant and add an allocation per packet.IpmiClientConfiguration: the passwordchar[]and BMC key are shared deliberately, so the caller can wipe them.Fru,Sensor: result holders.PropertiesManager.getInstance(): singleton.SpotBugs reports a suppression that matches nothing (
US_USELESS_SUPPRESSION_*), so these can't go stale silently.For reviewers
*ResponseDataand the FRU records, for the reason above. Easy to add if you prefer them.upgrading.md:AuthenticationAlgorithm.getKeyExchangeAuthenticationCode(byte[], byte[])andcheckKeyExchangeAuthenticationCode(byte[], byte[], byte[]);UdpMessenger.getSentPackets()removed;CONST1/CONST2private;decodePayloadthrowsIllegalArgumentExceptionon an empty payload.char[]end to end), Data races: handshake/state fields polled without volatile, listener lists iterated unsynchronized #96 (listener lists, polling) and PropertiesManager and connection.properties issues: dead cleaningFrequency, NPE on missing resource, unsynchronized lazy init, INFO log per lookup #98 (missing resource, lazy singleton) are left for their own PRs.configuration.mdstates the UTF-8 encoding.Testing
mvn verify sitepasses: 34 tests, 0 Checkstyle, PMD and CPD clean, SpotBugs 0.Rakp1Testcomputes the SIK independently from IPMI 2.0 §13.31 with a high-byte Kg and a non-ASCII user name and password. It fails on the old code.Illegal connection state: Rakp1Waiting, as documented).🤖 Generated with Claude Code