Don't abort the SDR repository walk on OEM or undecodable records

AGENTS.md

@@ -0,0 +1,37 @@
+# Instructions for AI Agents
+
+## Code format
+
+The project has not adopted the MetricsHub Eclipse formatter profile yet (see issue #118). Until it does, match the style of the file you are editing: the `org.metricshub.ipmi.client` packages use tabs, the `org.metricshub.ipmi.core` packages (forked from the Verax IPMI library) use 4 spaces. Do not reformat code you are not otherwise changing. Once `formatter-maven-plugin` is configured, simply run `mvn formatter:format` before committing.
+
+All Java source files under `src/main/java` must include the proper LGPL-3 license header (the `license-maven-plugin` check covers `main/java/**/*.java` only; tests, Markdown and resources carry no header). When you add a new source file, run `mvn license:update-file-header` before committing (and before building, since the build fails if a source file lacks the header).
+
+All public methods must have proper Javadoc. Check the output of Maven to identify issues with Javadoc and fix these issues.
+
+The library targets **Java 8** (`maven.compiler.release` is 8): no `var`, records, switch expressions, text blocks or APIs newer than Java 8 in `src/main`.
+
+## Build
+
+The project uses Maven to build. A full build is performed with `mvn verify site` (or `mvn clean verify site` when applicable). CI runs the same on JDK 17.
+
+@codex, please don't try to use `mvnw` (Maven Wrapper). Maven is already installed and runs perfectly well.
+
+## Test
+
+Whenever required, when you add code or when you modify code that is not covered with unit tests, add the corresponding unit tests. All tests must pass with `mvn test`. Don't use the `-q` (silent) option, as you want to see the result of successful tests. Tests are run with the Maven surefire plugin and results are stored in the ./target/surefire-reports directory.
+
+Unit tests must not depend on a real BMC. Exercising the RMCP+ session code against real hardware is done with throw-away harnesses outside the repository; never commit hostnames, user names or passwords of test systems. When testing against a real BMC, keep in mind that BMCs drop UDP replies under concurrent sessions: repeat a failing request before blaming the library, set a short per-message timeout with `IpmiConnector.setTimeout(handle, ms)`, and end harness `main` methods with `System.exit(0)` because the library leaves non-daemon threads behind after a timeout.
+
+## 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. They are not yet gated (issues #114, #115, #116, #117 track the clean-up). Do not add new violations: check the reports for the files you changed before committing and submitting your code. On JDK 21+ the SpotBugs plugin version inherited from the parent POM cannot read the JDK class files; run `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs` instead.
+
+## Documentation
+
+Always make sure that public API changes are properly documented in src/site/markdown/*.md and that README.md is always up-to-date. The published documentation is https://metricshub.org/ipmi-java (see issue #113 for the planned overhaul).
+
+## IPMI specifics
+
+- The protocol implementation lives in `org.metricshub.ipmi.core` (RMCP+, RAKP, SDR/FRU/SEL coders, connection state machine); the MetricsHub-facing API is `org.metricshub.ipmi.client` (`IpmiClient`, `IpmiClientConfiguration`, the runners and `IpmiResultConverter`).
+- Decoders must never abort a whole SDR repository walk or FRU decode on a record they do not model: skip, log and continue (IPMI 2.0 reserves SDR record types 0xC0-0xFF for OEM use and vendors do emit them).
+- Byte-level parsing must follow the IPMI 2.0 specification (section numbers are cited in the code and in the issues); when a finding is "by the spec" but not reproduced on real hardware, say so in the commit or PR.

CLAUDE.md

@@ -0,0 +1 @@
+@AGENTS.md

src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java

@@ -40,14 +40,18 @@
 import org.metricshub.ipmi.core.coding.security.CipherSuite;
 import org.metricshub.ipmi.core.common.TypeConverter;
 import org.metricshub.ipmi.core.connection.Connection;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * This abstract class implements common features required by FRUs, Sensor and Chassis Status runners.
- * 
- * @param <T> Represent the data type managed by the runner 
+ *
+ * @param <T> Represent the data type managed by the runner
  */
 public abstract class AbstractIpmiRunner<T> implements AutoCloseable, Callable<T> {
 
+	private static final Logger LOGGER = LoggerFactory.getLogger(AbstractIpmiRunner.class);
+
 	private static final int DEFAULT_LOCAL_UDP_PORT = 0;
 
 	/**
@@ -167,9 +171,9 @@
 			// Close the session
 			try {
 				connector.closeSession(handle);
 			} catch (Exception e) {
 				// Ignore
 			}
 		}
 
 		// Close connection manager and release the listener port.
@@ -192,13 +196,18 @@
 			GetSdrResponseData data = (GetSdrResponseData) connector.sendMessage(handle,
 					new GetSdr(IpmiVersion.V20, handle.getCipherSuite(), AuthenticationType.RMCPPlus, reservationId, nextRecId));
 
-			// If getting whole record succeeded we create SensorRecord from
-			// received data...
-			SensorRecord sensorDataToPopulate = SensorRecord.populateSensorRecord(data.getSensorRecordData());
+			// Some BMCs answer with success but return fewer bytes than the
+			// record length declared in the header: fall back to the chunked
+			// read while the current record ID is still known
+			if (isTruncated(data.getSensorRecordData())) {
+				return getSensorViaChunks(reservationId);
+			}
 
-			// ... and update the ID of the next record
+			// Advance to the next record first, so that a record we cannot
+			// decode never stalls the whole repository walk
 			nextRecId = data.getNextRecordId();
-			return sensorDataToPopulate;
+
+			return decodeRecord(data.getSensorRecordData());
 
 		} catch (IPMIException e) {
 
@@ -229,14 +238,22 @@
 		GetSdrResponseData data = (GetSdrResponseData) connector.sendMessage(handle, new GetSdr(IpmiVersion.V20, handle.getCipherSuite(),
 				AuthenticationType.RMCPPlus, reservationId, nextRecId, 0, INITIAL_CHUNK_SIZE));
 
+		byte[] header = data.getSensorRecordData();
+		if (header == null || header.length < HEADER_SIZE) {
+			LOGGER.warn("Skipping SDR record {} on {}: BMC returned {} byte(s) instead of the {}-byte header", nextRecId,
+					ipmiConfiguration.getHostname(), header == null ? 0 : header.length, HEADER_SIZE);
+			nextRecId = data.getNextRecordId();
+			return null;
+		}
+
 		// The record size is 5th byte of the record. It does not take
 		// into account the size of the header, so we need to add it.
-		int recSize = TypeConverter.byteToInt(data.getSensorRecordData()[4]) + HEADER_SIZE;
-		int read = INITIAL_CHUNK_SIZE;
+		int recSize = TypeConverter.byteToInt(header[HEADER_SIZE - 1]) + HEADER_SIZE;
+		int read = Math.min(header.length, recSize);
 
 		byte[] bytes = new byte[recSize];
 
-		System.arraycopy(data.getSensorRecordData(), 0, bytes, 0, data.getSensorRecordData().length);
+		System.arraycopy(header, 0, bytes, 0, read);
 
 		// We get the rest of the record in chunks (watch out for
 		// exceeding the record size, since this will result in BMC's
@@ -251,19 +268,54 @@
 			GetSdrResponseData part = (GetSdrResponseData) connector.sendMessage(handle, new GetSdr(IpmiVersion.V20, handle.getCipherSuite(),
 					AuthenticationType.RMCPPlus, reservationId, nextRecId, read, bytesToRead));
 
+			byte[] chunk = part.getSensorRecordData();
+			int got = chunk == null ? 0 : Math.min(bytesToRead, chunk.length);
+			if (got == 0) {
+				LOGGER.warn("Skipping SDR record {} on {}: BMC returned no data at offset {}", nextRecId,
+						ipmiConfiguration.getHostname(), read);
+				nextRecId = data.getNextRecordId();
+				return null;
+			}
+
 			// Append the new bytes
-			System.arraycopy(part.getSensorRecordData(), 0, bytes, read, bytesToRead);
+			System.arraycopy(chunk, 0, bytes, read, got);
 
-			read += bytesToRead;
+			read += got;
 		}
 
-		// Finally we populate the sensor record with the gathered
-		// data...
-		SensorRecord sensorDataToPopulate = SensorRecord.populateSensorRecord(bytes);
-
-		// ... and update the ID of the next record
+		// Advance to the next record, then decode the gathered data
 		nextRecId = data.getNextRecordId();
 
-		return sensorDataToPopulate;
+		return decodeRecord(bytes);
+	}
+
+	/**
+	 * Decode a raw SDR record. A record the library cannot model (unknown or malformed type) is logged and skipped
+	 * instead of aborting the whole repository walk.
+	 *
+	 * @param recordData Raw bytes of the SDR record
+	 * @return {@link SensorRecord} instance or <code>null</code> if the record cannot be decoded
+	 */
+	SensorRecord decodeRecord(byte[] recordData) {
+		try {
+			return SensorRecord.populateSensorRecord(recordData);
+		} catch (RuntimeException e) {
+			LOGGER.warn("Skipping undecodable SDR record before id {} on {}: {}", nextRecId, ipmiConfiguration.getHostname(), e.getMessage());
+			return null;
+		}
+	}
+
+	/**
+	 * Detect a whole-record GetSdr response that is shorter than the record length declared in its own header. Some BMCs
+	 * answer such requests with a success completion code and a truncated body instead of "Cannot return number of
+	 * requested data bytes".
+	 *
+	 * @param recordData Raw bytes returned for a whole-record GetSdr request
+	 * @return <code>true</code> if the header is missing or fewer bytes than declared were returned
+	 */
+	static boolean isTruncated(byte[] recordData) {
+		return recordData == null
+				|| recordData.length < HEADER_SIZE
+				|| recordData.length < TypeConverter.byteToInt(recordData[HEADER_SIZE - 1]) + HEADER_SIZE;
 	}
 }

src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/OemRecord.java

@@ -29,6 +29,16 @@
  */
 public class OemRecord extends SensorRecord {
 
+    /**
+     * Size of the common SDR record header (record ID, SDR version, record type, record length).
+     */
+    private static final int HEADER_LENGTH = 5;
+
+    /**
+     * Size of the manufacturer ID field in a C0h OEM record.
+     */
+    private static final int MANUFACTURER_ID_LENGTH = 3;
+
     private int manufacturerId;
 
     private byte[] oemData;
@@ -37,17 +47,24 @@
     protected void populateTypeSpecficValues(byte[] recordData,
             SensorRecord record) {
 
-        byte[] buffer = new byte[4];
-
-        System.arraycopy(recordData, 5, buffer, 0, 3);
-
-        buffer[3] = 0;
+        // Only the C0h OEM record has a defined layout (3-byte manufacturer ID
+        // followed by OEM data). Vendor-defined record types C1h-FFh carry an
+        // unknown layout, so keep their whole type-specific payload and leave
+        // the manufacturer ID at 0 (unknown).
+        int dataOffset = HEADER_LENGTH;
 
-        setManufacturerId(TypeConverter.littleEndianByteArrayToInt(buffer));
+        if (recordData[3] == RecordTypes.OEM_RECORD && recordData.length >= HEADER_LENGTH + MANUFACTURER_ID_LENGTH) {
+            byte[] buffer = new byte[4];
+            System.arraycopy(recordData, HEADER_LENGTH, buffer, 0, MANUFACTURER_ID_LENGTH);
+            setManufacturerId(TypeConverter.littleEndianByteArrayToInt(buffer));
+            dataOffset += MANUFACTURER_ID_LENGTH;
+        }
 
-        byte[] data = new byte[recordData.length - 8];
+        byte[] data = new byte[Math.max(0, recordData.length - dataOffset)];
 
-        System.arraycopy(recordData, 8, data, 0, data.length);
+        if (data.length > 0) {
+            System.arraycopy(recordData, dataOffset, data, 0, data.length);
+        }
 
         setOemData(data);
     }
@@ -61,11 +78,11 @@
     }
 
     public byte[] getOemData() {
         return oemData;
     }
 
     public void setOemData(byte[] oemData) {
         this.oemData = oemData;
     }
 
 }

src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecord.java

@@ -91,6 +91,13 @@ public static SensorRecord populateSensorRecord(byte[] recordData) {
             sensorRecord = new OemRecord();
             break;
         default:
+            // IPMI 2.0 Table 43-1 reserves C0h-FFh for OEM records; vendors use
+            // values above C0h (e.g. NVIDIA/GIGABYTE emit D0h). Never abort the
+            // SDR repository walk on a record type we don't model.
+            if (TypeConverter.byteToInt(recType) >= TypeConverter.byteToInt(RecordTypes.OEM_RECORD)) {
+                sensorRecord = new OemRecord();
+                break;
+            }
             throw new IllegalArgumentException("Invalid record type: "
                     + recType);
         }

src/test/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunnerTest.java

@@ -0,0 +1,74 @@
+package org.metricshub.ipmi.client.runner;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+import org.metricshub.ipmi.client.IpmiClientConfiguration;
+import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord;
+import org.metricshub.ipmi.core.coding.commands.sdr.record.OemRecord;
+
+class AbstractIpmiRunnerTest {
+
+	/**
+	 * Minimal concrete runner: no session, no connector, just the decoding helpers under test.
+	 */
+	private static final AbstractIpmiRunner<Void> RUNNER = new AbstractIpmiRunner<Void>(
+			new IpmiClientConfiguration("bmc", "user", new char[0], null, false, 1)) {
+		@Override
+		public Void call() {
+			return null;
+		}
+	};
+
+	private static byte[] record(int type, byte[] payload) {
+		byte[] raw = new byte[5 + payload.length];
+		raw[2] = 0x51;
+		raw[3] = (byte) type;
+		raw[4] = (byte) payload.length;
+		System.arraycopy(payload, 0, raw, 5, payload.length);
+		return raw;
+	}
+
+	@Test
+	void decodeRecordReturnsOemRecordForVendorDefinedType() {
+		assertInstanceOf(OemRecord.class, RUNNER.decodeRecord(record(0xd0, new byte[] {1, 2, 3})));
+	}
+
+	@Test
+	void decodeRecordSkipsRecordsItCannotDecodeInsteadOfThrowing() {
+		// reserved (non-OEM) record type
+		assertNull(RUNNER.decodeRecord(record(0x05, new byte[] {1})));
+		// shorter than the SDR header
+		assertNull(RUNNER.decodeRecord(new byte[] {0x00}));
+		// full sensor record whose body is missing: decoder runs out of bytes
+		assertNull(RUNNER.decodeRecord(record(0x01, new byte[] {1, 2, 3})));
+	}
+
+	@Test
+	void decodeRecordStillDecodesValidRecords() {
+		// A Full Sensor Record (type 01h) of minimal valid size: 48 bytes, the last one being the ID string type/length
+		byte[] payload = new byte[43];
+		payload[0] = 0x20; // sensor owner: BMC
+		payload[7] = 0x01; // sensor type: temperature
+		payload[8] = 0x01; // event/reading type: threshold
+		payload[16] = 0x01; // base unit: degrees C
+		payload[42] = (byte) 0xc0; // 8-bit ASCII, zero-length name
+		assertInstanceOf(FullSensorRecord.class, RUNNER.decodeRecord(record(0x01, payload)));
+	}
+
+	@Test
+	void isTruncatedDetectsWholeRecordResponsesShorterThanDeclared() {
+		byte[] complete = record(0xd0, new byte[] {1, 2, 3, 4, 5});
+		assertFalse(AbstractIpmiRunner.isTruncated(complete));
+
+		byte[] truncated = new byte[complete.length - 2];
+		System.arraycopy(complete, 0, truncated, 0, truncated.length);
+		assertTrue(AbstractIpmiRunner.isTruncated(truncated), "declared length 5 but only 3 payload bytes returned");
+
+		assertTrue(AbstractIpmiRunner.isTruncated(new byte[] {0x00, 0x00, 0x51}), "header itself is incomplete");
+		assertTrue(AbstractIpmiRunner.isTruncated(null));
+	}
+}

src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecordTest.java

@@ -0,0 +1,82 @@
+package org.metricshub.ipmi.core.coding.commands.sdr.record;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import org.junit.jupiter.api.Test;
+
+class SensorRecordTest {
+
+	/**
+	 * Build a raw SDR record: 2-byte record ID (LE), SDR version 0x51, record type, record length, then the payload.
+	 */
+	private static byte[] record(int id, int type, byte[] payload) {
+		byte[] raw = new byte[5 + payload.length];
+		raw[0] = (byte) (id & 0xff);
+		raw[1] = (byte) ((id >> 8) & 0xff);
+		raw[2] = 0x51;
+		raw[3] = (byte) type;
+		raw[4] = (byte) payload.length;
+		System.arraycopy(payload, 0, raw, 5, payload.length);
+		return raw;
+	}
+
+	@Test
+	void vendorDefinedRecordTypeIsDecodedAsOemRecordWithWholePayload() {
+		// Record type D0h as emitted by GIGABYTE/NVIDIA BMCs: vendor-defined layout, no manufacturer ID field
+		byte[] payload = {0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
+
+		SensorRecord record = SensorRecord.populateSensorRecord(record(0x0004, 0xd0, payload));
+
+		OemRecord oem = assertInstanceOf(OemRecord.class, record);
+		assertEquals(4, oem.getId());
+		assertEquals(0, oem.getManufacturerId(), "manufacturer ID is unknown for vendor-defined record types");
+		assertArrayEquals(payload, oem.getOemData(), "the complete type-specific payload must be preserved");
+	}
+
+	@Test
+	void lastOemRecordTypeIsAccepted() {
+		byte[] payload = {0x01, 0x02};
+
+		OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0010, 0xff, payload)));
+
+		assertArrayEquals(payload, oem.getOemData());
+	}
+
+	@Test
+	void standardOemRecordTypeC0ParsesManufacturerIdAndData() {
+		// C0h layout: 3-byte manufacturer ID (LE, 0x000157 = 343) followed by OEM data
+		byte[] payload = {0x57, 0x01, 0x00, 0x21, 0x22};
+
+		OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0020, 0xc0, payload)));
+
+		assertEquals(343, oem.getManufacturerId());
+		assertArrayEquals(new byte[] {0x21, 0x22}, oem.getOemData());
+	}
+
+	@Test
+	void shortOemRecordDoesNotThrow() {
+		// C0h record without even a complete manufacturer ID: no ID is fabricated and the bytes are kept as payload
+		OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0030, 0xc0, new byte[] {0x57})));
+
+		assertEquals(0, oem.getManufacturerId());
+		assertArrayEquals(new byte[] {0x57}, oem.getOemData());
+
+		// header only
+		OemRecord empty = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0031, 0xc0, new byte[0])));
+		assertEquals(0, empty.getOemData().length);
+	}
+
+	@Test
+	void unknownStandardRecordTypeIsRejected() {
+		// 05h is reserved by IPMI 2.0 and not an OEM type: callers are expected to skip it
+		assertThrows(IllegalArgumentException.class, () -> SensorRecord.populateSensorRecord(record(0x0040, 0x05, new byte[] {0x00})));
+	}
+
+	@Test
+	void recordShorterThanHeaderIsRejected() {
+		assertThrows(IllegalArgumentException.class, () -> SensorRecord.populateSensorRecord(new byte[] {0x00, 0x00, 0x51}));
+	}
+}