Fix Crc16.update(ByteBuffer) reading past limit for positioned buffers

[youdie006]

Summary

Fixes #157. Crc16.update(ByteBuffer) throws IndexOutOfBoundsException for any buffer whose position is non-zero.

Root cause

int offset = buffer.position();
for (int i = offset; i < buffer.limit(); i++) {
  update(buffer.get(offset + i));   // offset added twice
}

i already starts at offset (the position), so buffer.get(offset + i) reads the buffer at doubled indices. When offset > 0, offset + i runs off the end and ByteBuffer.get(int) throws once the index reaches limit(). The bug is masked only when position() == 0 (offset + i == i), which is why the existing callers that pass position-0 buffers were unaffected.

Fix

Index each absolute position i in [position, limit) exactly once:

int offset = buffer.position();
for (int i = offset; i < buffer.limit(); i++) {
  update(buffer.get(i));
}

This matches the method's documented contract of CRC-ing the buffer's remaining bytes and does not advance the buffer position (absolute get).

Test

Added Crc16Test.crc16WithBufferPosition, which prefixes the existing known CRC vector with two bytes and advances position past them; the CRC over the remaining bytes must equal the same known value (0x2590).

• Before the fix: IndexOutOfBoundsException at Crc16.java.
• After the fix: passes.

Verification

• mvn -pl modbus test — 79 tests, 0 failures/errors (the RTU frame accumulator paths that CRC positioned buffers are covered).
• mvn -pl modbus checkstyle:check — 0 violations.
• mvn -pl modbus spotless:check — clean.

---

Disclosure: this fix was prepared with AI assistance (Claude). I have reviewed it and verified the reasoning, the red-green test, and the full module build myself.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Corrected CRC calculations when processing a buffer whose starting position has advanced.
  • CRC results now consistently cover only the remaining bytes in the buffer.

• Tests

  • Added regression coverage for CRC calculations on partially consumed buffers.

Walkthrough

Crc16.update(ByteBuffer) now processes bytes from the buffer’s current position to its limit without double-adding the position. A regression test verifies the CRC for a positioned buffer.

Changes

CRC16 ByteBuffer handling

Layer / File(s)	Summary
Correct ByteBuffer range processing modbus/src/main/java/com/digitalpetri/modbus/Crc16.java, modbus/src/test/java/com/digitalpetri/modbus/Crc16Test.java	The update loop reads the buffer’s readable range directly. The test verifies CRC 0x2590 after advancing the buffer position.
Estimated code review effort: 2 (Simple)	~5 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The code fixes the double-position bug, but the provided test summary does not show coverage for limit-below-capacity and empty ranges required by #157.	Add regression tests for a positioned buffer with limit below capacity and for a buffer where position equals limit, then verify both cases.
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the fix for Crc16.update(ByteBuffer) with positioned buffers.
Description check	✅ Passed	The description explains the defect, root cause, fix, regression test, and verification steps.
Out of Scope Changes check	✅ Passed	The changes are limited to the Crc16 ByteBuffer fix and its regression test, which are within the scope of issue #157.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modbus/src/test/java/com/digitalpetri/modbus/Crc16Test.java`:
- Around line 20-30: Extend crc16WithBufferPosition to cover a ByteBuffer with
limit below capacity, an empty range where position equals limit, and
preservation of the original buffer.position() after crc.update(buffer). Assert
the expected CRC for the bounded remaining bytes, zero for the empty range, and
the unchanged position for each relevant case.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eeaf114f-be2a-42e6-92ac-848901f7c3a8

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff26c0 and c5aca12.

📒 Files selected for processing (2)

• modbus/src/main/java/com/digitalpetri/modbus/Crc16.java
• modbus/src/test/java/com/digitalpetri/modbus/Crc16Test.java

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the remaining ByteBuffer boundary tests.

The current test uses a buffer whose limit equals its capacity. It does not cover a limit below capacity, an empty [position, limit) range, or preservation of buffer.position().

Add these assertions and cases to match the PR objectives:

Proposed test additions

     crc.update(buffer);
 
+    assertEquals(2, buffer.position());
     assertEquals(0x2590, crc.getValue());
   }
 
+  `@Test`
+  void crc16WithLimitBelowCapacity() {
+    Crc16 crc = new Crc16();
+    ByteBuffer buffer =
+        ByteBuffer.wrap(new byte[] {0x00, 0x00, 0x12, 0x34, 0x56, 0x78, 0x09, 0x7F});
+    buffer.position(2);
+    buffer.limit(7);
+
+    crc.update(buffer);
+
+    assertEquals(0x2590, crc.getValue());
+    assertEquals(2, buffer.position());
+  }
+
+  `@Test`
+  void crc16WithEmptyRemainingRange() {
+    Crc16 crc = new Crc16();
+    ByteBuffer buffer = ByteBuffer.allocate(8);
+    buffer.position(4);
+    buffer.limit(4);
+
+    crc.update(buffer);
+
+    assertEquals(0xFFFF, crc.getValue());
+    assertEquals(4, buffer.position());
+  }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	@Test
	void crc16WithBufferPosition() {
	// A buffer whose position is advanced past leading bytes must be CRC'd
	// over only the remaining bytes [position, limit). Prefixing the known
	// vector with two bytes and skipping them must yield the same CRC. See #157.
	Crc16 crc = new Crc16();
	ByteBuffer buffer = ByteBuffer.wrap(new byte[] {0x00, 0x00, 0x12, 0x34, 0x56, 0x78, 0x09});
	buffer.position(2);
	crc.update(buffer);
	assertEquals(0x2590, crc.getValue());
	`@Test`
	void crc16WithBufferPosition() {
	// A buffer whose position is advanced past leading bytes must be CRC'd
	// over only the remaining bytes [position, limit). Prefixing the known
	// vector with two bytes and skipping them must yield the same CRC. See `#157`.
	Crc16 crc = new Crc16();
	ByteBuffer buffer = ByteBuffer.wrap(new byte[] {0x00, 0x00, 0x12, 0x34, 0x56, 0x78, 0x09});
	buffer.position(2);
	crc.update(buffer);
	assertEquals(2, buffer.position());
	assertEquals(0x2590, crc.getValue());
	}
	`@Test`
	void crc16WithLimitBelowCapacity() {
	Crc16 crc = new Crc16();
	ByteBuffer buffer =
	ByteBuffer.wrap(new byte[] {0x00, 0x00, 0x12, 0x34, 0x56, 0x78, 0x09, 0x7F});
	buffer.position(2);
	buffer.limit(7);
	crc.update(buffer);
	assertEquals(0x2590, crc.getValue());
	assertEquals(2, buffer.position());
	}
	`@Test`
	void crc16WithEmptyRemainingRange() {
	Crc16 crc = new Crc16();
	ByteBuffer buffer = ByteBuffer.allocate(8);
	buffer.position(4);
	buffer.limit(4);
	crc.update(buffer);
	assertEquals(0xFFFF, crc.getValue());
	assertEquals(4, buffer.position());
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modbus/src/test/java/com/digitalpetri/modbus/Crc16Test.java` around lines 20
- 30, Extend crc16WithBufferPosition to cover a ByteBuffer with limit below
capacity, an empty range where position equals limit, and preservation of the
original buffer.position() after crc.update(buffer). Assert the expected CRC for
the bounded remaining bytes, zero for the empty range, and the unchanged
position for each relevant case.