Fix the SpotBugs findings and gate the build on SpotBugs (#116)

AGENTS.md

@@ -26,7 +26,7 @@ Unit tests must not depend on a real BMC. Exercising the RMCP+ session code agai
 
 ## Code quality reports
 
-Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD is gated on duplications of 100 tokens or more (`cpd-check` at `verify`), while the site report lists those of 50 tokens or more; SpotBugs is not yet gated (issue #116 tracks the clean-up). Extract shared code instead of copying it: `AbstractSensorRecord` holds what the Full, Compact and Event-Only sensor records share, and `IpmiCommandCoder.validateResponse()` the response checks of every command. Do not add new violations: check the reports for the files you changed before committing and submitting your code. The SpotBugs site report runs spotbugs-maven-plugin 4.10.4.1 (pinned in `<reporting>`, as the 4.9.3.0 of the parent POM cannot read the class files of JDK 21+); to run SpotBugs alone, use `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs`.
+Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD is gated on duplications of 100 tokens or more (`cpd-check` at `verify`), while the site report lists those of 50 tokens or more; SpotBugs is gated on any bug at the default threshold (`spotbugs:check` at `verify`). Extract shared code instead of copying it: `AbstractSensorRecord` holds what the Full, Compact and Event-Only sensor records share, and `IpmiCommandCoder.validateResponse()` the response checks of every command. Do not add new violations: check the reports for the files you changed before committing and submitting your code. The SpotBugs gate and site report run spotbugs-maven-plugin 4.10.4.1 (pinned in `<build>` and `<reporting>`, as the 4.9.3.0 of the parent POM cannot read the class files of JDK 21+); to run SpotBugs alone, use `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:check`. Fix SpotBugs findings; when one is intentional, suppress it with `@SuppressFBWarnings` (`spotbugs-annotations`, provided scope) and a `justification`. `CT_CONSTRUCTOR_THROW` and `EI_EXPOSE_REP` (which also matches `EI_EXPOSE_REP2`) are suppressed for the whole protocol core in `org/metricshub/ipmi/core/package-info.java`; SpotBugs reports a suppression that matches nothing, so remove it with the code it covered.
 
 ## Documentation
 

README.md

@@ -20,7 +20,7 @@ The BMC must have IPMI over LAN enabled and an account with the User privilege:
 
 ## Upgrading
 
-Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page.
+Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`.
 
 ## Build instructions
 
@@ -42,6 +42,8 @@ mvn formatter:format
 
 The build also fails on [Checkstyle](checkstyle.xml) violations. A justified violation can be suppressed with `// CHECKSTYLE.OFF: <RuleName>` and `// CHECKSTYLE.ON: <RuleName>` comments.
 
+The build fails on any SpotBugs bug as well. An intentional one is suppressed with `@SuppressFBWarnings` and a `justification`.
+
 To ignore the whole-tree reformat commit in `git blame`, run once:
 
 ```bash

pom.xml

@@ -92,6 +92,13 @@
 	</dependencyManagement>
 
 	<dependencies>
+		<!-- @SuppressFBWarnings for the justified SpotBugs suppressions; compile-time only (provided scope) -->
+		<dependency>
+			<groupId>com.github.spotbugs</groupId>
+			<artifactId>spotbugs-annotations</artifactId>
+			<version>4.10.4</version>
+			<scope>provided</scope>
+		</dependency>
 		<dependency>
 			<groupId>org.slf4j</groupId>
 			<artifactId>slf4j-api</artifactId>
@@ -177,6 +184,22 @@
 				</executions>
 			</plugin>
 
+			<!-- spotbugs: fail the build on any SpotBugs finding; same version as the site report below. The 4.9.3.0
+				inherited from oss-parent cannot read the class files of JDK 21+ -->
+			<plugin>
+				<groupId>com.github.spotbugs</groupId>
+				<artifactId>spotbugs-maven-plugin</artifactId>
+				<version>4.10.4.1</version>
+				<executions>
+					<execution>
+						<phase>verify</phase>
+						<goals>
+							<goal>check</goal>
+						</goals>
+					</execution>
+				</executions>
+			</plugin>
+
 			<!-- license: add headers to new files, never rewrite existing ones. The parent's canUpdateCopyright would turn
 				every "Copyright 2023 Verax Systems, MetricsHub" into "Copyright 2023 - <current year> MetricsHub", and its
 				canUpdateDescription makes every header look outdated on Windows (CRLF vs LF), which re-renders the copyright
@@ -219,7 +242,7 @@
 				<version>3.6.0</version>
 			</plugin>
 
-			<!-- spotbugs: the 4.9.3.0 inherited from oss-parent cannot read the class files of JDK 21+ -->
+			<!-- spotbugs: same version as the build gate, so the report shows what the gate checked -->
 			<plugin>
 				<groupId>com.github.spotbugs</groupId>
 				<artifactId>spotbugs-maven-plugin</artifactId>

src/main/java/org/metricshub/ipmi/client/IpmiClientConfiguration.java

@@ -24,10 +24,15 @@
 
 import org.metricshub.ipmi.core.common.Constants;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * IPMI configuration including the required credentials that need to be used to establish the
  * communication with the IPMI interface.
  */
+// EI_EXPOSE_REP also matches EI_EXPOSE_REP2
+@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "The password char[] and BMC key byte[] are "
+		+ "deliberately shared, not copied, so that the caller can wipe the only copy of the credentials")
 public class IpmiClientConfiguration {
 
 	private String hostname;

src/main/java/org/metricshub/ipmi/client/model/Fru.java

@@ -24,6 +24,8 @@
 
 import java.util.List;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import org.metricshub.ipmi.core.coding.commands.fru.record.BoardInfo;
 import org.metricshub.ipmi.core.coding.commands.fru.record.ChassisInfo;
 import org.metricshub.ipmi.core.coding.commands.fru.record.FruRecord;
@@ -37,6 +39,9 @@
  * <li>The FRU records containing {@link BoardInfo}, {@link ChassisInfo} and/or {@link ProductInfo}.</li>
  * </ul>
  */
+// EI_EXPOSE_REP also matches EI_EXPOSE_REP2
+@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Result holder: hands out the decoded records, "
+		+ "which are mutable holders themselves, by reference")
 public class Fru {
 
 	private FruDeviceLocatorRecord fruLocator;

src/main/java/org/metricshub/ipmi/client/model/Sensor.java

@@ -22,6 +22,8 @@
  * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱
  */
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.AbstractSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord;
@@ -42,6 +44,9 @@
  * <em>$sensorName=$state|$sensorName=$state|...|$sensorName=$state</em></li>
  * </ul>
  */
+// EI_EXPOSE_REP also matches EI_EXPOSE_REP2
+@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Result holder: hands out the decoded record and "
+		+ "reading, which are mutable holders themselves, by reference")
 public class Sensor {
 
 	private SensorRecord sensorRecord;

src/main/java/org/metricshub/ipmi/core/api/sol/SerialOverLan.java

@@ -413,7 +413,7 @@ public boolean writeIntArray(int[] buffer) {
 	 * @return true if whole string was successfully sent and acknowledged by remote server, false otherwise.
 	 */
 	public boolean writeString(String string) {
-		return writeBytes(string.getBytes());
+		return writeString(string, Charset.defaultCharset());
 	}
 
 	/**
@@ -541,7 +541,7 @@ public String readString() {
 	 * @return all bytes that could be read as {@link String}, but no more than given byteCount.
 	 */
 	public String readString(int byteCount) {
-		return new String(readBytes(byteCount));
+		return readString(Charset.defaultCharset(), byteCount);
 	}
 
 	/**

src/main/java/org/metricshub/ipmi/core/api/sync/MessageListener.java

@@ -46,7 +46,7 @@ public class MessageListener implements IpmiResponseListener {
 
 	private int tag;
 
-	private IpmiResponse response;
+	private volatile IpmiResponse response;
 
 	/**
 	 * Messages that have proper connection handle but arrived before tag was
@@ -86,17 +86,21 @@ public ResponseData waitForAnswer(int messageTag) throws Exception {
 		if (messageTag < 0 || messageTag > 63) {
 			throw new IllegalArgumentException("Corrupted message tag");
 		}
-		this.tag = messageTag;
-		for (IpmiResponse quickResponse : quickMessages) {
-			this.notify(quickResponse);
+		synchronized (this) {
+			this.tag = messageTag;
+			for (IpmiResponse quickResponse : quickMessages) {
+				this.notify(quickResponse);
+			}
 		}
 
 		while (response == null) {
 			Thread.sleep(1);
 		}
 		if (response instanceof IpmiResponseData) {
-			this.tag = -1;
-			quickMessages.clear();
+			synchronized (this) {
+				this.tag = -1;
+				quickMessages.clear();
+			}
 			return ((IpmiResponseData) response).getResponseData();
 		} else /* response instanceof IpmiError */ {
 			throw ((IpmiError) response).getException();

src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java

@@ -24,7 +24,6 @@
 
 import org.metricshub.ipmi.core.coding.PayloadCoder;
 import org.metricshub.ipmi.core.coding.payload.CompletionCode;
-import org.metricshub.ipmi.core.coding.payload.IpmiPayload;
 import org.metricshub.ipmi.core.coding.payload.PlainMessage;
 import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse;
@@ -66,15 +65,10 @@ public PayloadType getSupportedPayloadType() {
 	 *         class, false otherwise.
 	 */
 	public boolean isCommandResponse(IpmiMessage message) {
-		if (message.getPayload() instanceof IpmiPayload) {
-			if (message.getPayload() instanceof IpmiLanResponse) {
-				return ((IpmiLanResponse) message.getPayload()).getCommand() == getCommandCode();
-			} else {
-				return message.getPayload() instanceof PlainMessage;
-			}
-		} else {
-			return false;
+		if (message.getPayload() instanceof IpmiLanResponse) {
+			return ((IpmiLanResponse) message.getPayload()).getCommand() == getCommandCode();
 		}
+		return message.getPayload() instanceof PlainMessage;
 	}
 
 	/**

src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisInfo.java

@@ -105,9 +105,6 @@ public ChassisInfo(final byte[] fruData, final int offset) {
 									true));
 					break;
 				default:
-					if (partDataLength == 0) {
-						continue;
-					}
 					customInfo
 							.add(
 									decodeString(

src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessInfo.java

@@ -24,6 +24,8 @@
 
 import org.metricshub.ipmi.core.common.TypeConverter;
 
+import java.nio.charset.StandardCharsets;
+
 /**
  * Management Access Information record from FRU Multi Record Area
  */
@@ -45,7 +47,7 @@
 	 */
 	public ManagementAccessInfo(byte[] fruData, int offset, int length) {
 		super();
 		// TODO: Test when server containing such records will be available
 
 		recordType = ManagementAccessRecordType
 				.parseInt(
@@ -56,7 +58,7 @@
 
 		System.arraycopy(fruData, offset + 1, buffer, 0, length - 1);
 
-		accessInfo = new String(buffer);
+		accessInfo = new String(buffer, StandardCharsets.ISO_8859_1);
 	}
 
 	public ManagementAccessRecordType getRecordType() {