Fix offset bugs in MessagePackGenerator and document pre-existing issues

Three bugs where non-zero offset/position was ignored:
- getBytesIfAscii: bytes[i] -> bytes[i - offset]
- writeByteArrayTextValue: store slice instead of whole array
- ByteBuffer: use arrayOffset() + position() as payload start

All three bugs exist identically in msgpack-jackson; documented in
plans/preexisting-issues.md with practical impact notes.

Author: komamitsu

msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java

@@ -390,7 +390,7 @@ else if (v instanceof ByteBuffer) {
             int len = bb.remaining();
             if (bb.hasArray()) {
                 messagePacker.packBinaryHeader(len);
-                messagePacker.writePayload(bb.array(), bb.arrayOffset(), len);
+                messagePacker.writePayload(bb.array(), bb.arrayOffset() + bb.position(), len);
             }
             else {
                 byte[] data = new byte[len];
@@ -496,7 +496,7 @@ private byte[] getBytesIfAscii(char[] chars, int offset, int len)
             if (c >= 0x80) {
                 return null;
             }
-            bytes[i] = (byte) c;
+            bytes[i - offset] = (byte) c;
         }
         return bytes;
     }
@@ -524,7 +524,9 @@ private void writeCharArrayTextValue(char[] text, int offset, int len) throws IO
     private void writeByteArrayTextValue(byte[] text, int offset, int len) throws IOException
     {
         if (areAllAsciiBytes(text, offset, len)) {
-            addValueNode(new AsciiCharString(text));
+            byte[] slice = new byte[len];
+            System.arraycopy(text, offset, slice, 0, len);
+            addValueNode(new AsciiCharString(slice));
             return;
         }
         addValueNode(new String(text, offset, len, DEFAULT_CHARSET));

msgpack-jackson3/src/test/java/org/msgpack/jackson/dataformat/MessagePackGeneratorTest.java

@@ -956,4 +956,97 @@ public String getName()
             return name;
         }
     }
+
+    @Test
+    public void testWriteStringCharArrayWithOffset()
+            throws IOException
+    {
+        // Padding chars before/after the actual content to test non-zero offset
+        char[] buf = new char[] {'X', 'X', 'h', 'e', 'l', 'l', 'o', 'X'};
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
+        generator.writeStartArray();
+        generator.writeString(buf, 2, 5); // "hello"
+        generator.writeEndArray();
+        generator.close();
+
+        MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
+        unpacker.unpackArrayHeader();
+        assertEquals("hello", unpacker.unpackString());
+    }
+
+    @Test
+    public void testWriteStringCharArrayWithOffsetNonAscii()
+            throws IOException
+    {
+        // Non-ASCII to exercise the non-fast-path in getBytesIfAscii
+        char[] buf = new char[] {'X', '三', '四', '五', 'X'}; // 三四五
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
+        generator.writeStartArray();
+        generator.writeString(buf, 1, 3);
+        generator.writeEndArray();
+        generator.close();
+
+        MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
+        unpacker.unpackArrayHeader();
+        assertEquals("三四五", unpacker.unpackString());
+    }
+
+    @Test
+    public void testWriteUTF8StringWithOffset()
+            throws IOException
+    {
+        // Padding bytes before/after to test non-zero offset in writeUTF8String
+        byte[] buf = new byte[] {'X', 'X', 'h', 'i', 'X'};
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
+        generator.writeStartArray();
+        generator.writeUTF8String(buf, 2, 2); // "hi"
+        generator.writeEndArray();
+        generator.close();
+
+        MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
+        unpacker.unpackArrayHeader();
+        assertEquals("hi", unpacker.unpackString());
+    }
+
+    @Test
+    public void testWriteBinaryWithOffset()
+            throws IOException
+    {
+        byte[] data = new byte[] {0x00, 0x01, 0x02, 0x03, 0x04};
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
+        generator.writeStartArray();
+        generator.writeBinary(data, 1, 3); // bytes 0x01, 0x02, 0x03
+        generator.writeEndArray();
+        generator.close();
+
+        MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
+        unpacker.unpackArrayHeader();
+        byte[] result = unpacker.readPayload(unpacker.unpackBinaryHeader());
+        assertArrayEquals(new byte[] {0x01, 0x02, 0x03}, result);
+    }
+
+    @Test
+    public void testWriteBinaryByteBufferWithOffset()
+            throws IOException
+    {
+        byte[] data = new byte[] {0x00, 0x01, 0x02, 0x03, 0x04};
+        ByteBuffer bb = ByteBuffer.wrap(data, 1, 3); // position=1, limit=4, remaining=3
+
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
+        generator.writeStartArray();
+        ObjectMapper mapper = new MessagePackMapper(factory);
+        mapper.writeValue(generator, bb);
+        generator.writeEndArray();
+        generator.close();
+
+        MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
+        unpacker.unpackArrayHeader();
+        byte[] result = unpacker.readPayload(unpacker.unpackBinaryHeader());
+        assertArrayEquals(new byte[] {0x01, 0x02, 0x03}, result);
+    }
 }

plans/preexisting-issues.md

@@ -0,0 +1,150 @@
+# Issues to address
+
+## msgpack-jackson3-specific
+
+### 1. `isClosed()` always returns false
+
+**File:** `msgpack-jackson3/.../MessagePackGenerator.java` (close method)
+
+`close()` cannot call `super.close()` because `GeneratorBase.close()` in Jackson 3
+closes the underlying output stream as a side effect, breaking tests that disable
+`AUTO_CLOSE_TARGET`. As a result `isClosed()` always returns false and callers can
+continue writing into a closed generator without getting the standard exception.
+
+### 2. `MessagePackFactory.snapshot()` returns `this` — FIXED
+
+**File:** `msgpack-jackson3/.../MessagePackFactory.java`
+
+Fixed: `snapshot()` now delegates to `copy()`, and `rebuild()` is implemented via `MessagePackFactoryBuilder`.
+
+### 3. Build: `msgpack-jackson3` fails to compile locally on Java < 17
+
+`msgpack-jackson3` is in the root aggregate unconditionally. The CI works around
+this with a bash version check, but a developer running `./sbt test` locally on
+Java 8 or 11 gets a hard compilation failure. A cleaner build-level solution
+(conditional aggregate, toolchain support, or a separate profile) is needed.
+
+### 4. `MessagePackGenerator.streamWriteContext()` returns null
+
+**File:** `msgpack-jackson3/.../MessagePackGenerator.java` (streamWriteContext method)
+
+`streamWriteContext()` returns `null`, bypassing Jackson's standard write context
+management. This can cause NPEs in Jackson code paths that use the context for path
+tracking in error messages or certain serialization features. Fixing it properly
+requires integrating with Jackson 3's `TokenStreamContext` / `_streamWriteContext`
+managed by `GeneratorBase`, which needs investigation.
+
+### 5. `writeString(Reader, int)` len=-1 implementation allocates an extra copy
+
+**File:** `msgpack-jackson3/.../MessagePackGenerator.java` (writeString(Reader, int))
+
+The len=-1 path buffers into a `StringBuilder` then copies to a `char[]`. Using a
+`CharArrayWriter` would avoid the intermediate allocation.
+
+---
+
+## msgpack-jackson pre-existing issues
+
+These issues were identified during review of msgpack-jackson3 and confirmed to exist
+identically in msgpack-jackson. They should be addressed in both modules together.
+
+## 1. `MessagePackParser`: Same byte-array input skips unpacker reset
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java:130`
+
+When `AUTO_CLOSE_SOURCE` is disabled and the same byte-array instance is parsed more
+than once (e.g. reused buffer), the condition `messageUnpackerTuple.first() != src`
+is false, so the unpacker is not reset. The second parse continues from where the first
+left off instead of from the beginning.
+
+## 2. `MessagePackParser`: ThreadLocal retains last byte-array payload per thread
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java:135`
+
+`messageUnpackerHolder` is never cleared on parser close. For byte-array inputs this
+retains the entire last parsed payload for each thread in a pool indefinitely, which
+can cause unbounded memory retention after large messages.
+
+## 3. `MessagePackGenerator`: `close()` does not call `super.close()`
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (close method)
+
+The `close()` override never delegates to `GeneratorBase.close()`, so `isClosed()`
+remains false after close. Callers can continue writing into a closed generator
+instead of getting the standard closed-generator exception.
+
+## 4. `MessagePackGenerator`: `writeString(Reader, int)` crashes on length -1
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (writeString(Reader, int))
+
+`new char[len]` throws `NegativeArraySizeException` when `len` is -1, which is a
+valid Jackson API usage meaning "unknown length, read until EOF".
+
+## 5. `MessagePackGenerator`: `writeNumber(String)` tries `Double.parseDouble` before `BigInteger`
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java:699`
+
+Integer strings outside the `long` range are serialized as floating-point values,
+losing precision, even though MessagePack can encode big integers exactly. The order
+should try integer parsing first.
+
+## 6. `MessagePackGenerator`: Closing a container leaves stale `currentState`
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (writeEndArray/writeEndObject)
+
+After `flush()` clears `nodes`, any subsequent root-level value written with the
+same generator is treated as if inside the old container, which can corrupt output.
+
+## 7. `MessagePackSerializedString`: Most interface methods are stubs
+
+**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackSerializedString.java:70`
+
+Most `SerializableString` methods return 0 or do nothing. Any Jackson code path
+that calls these methods (e.g. for length or byte-copy operations) will silently
+produce incorrect results.
+
+## 8. `MessagePackGenerator`: `getBytesIfAscii` writes to wrong index when offset > 0
+
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java:474`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
+
+`bytes[i] = (byte) c` should be `bytes[i - offset] = (byte) c`. When offset > 0, `i`
+starts above 0 but `bytes` is length `len`, so it throws `ArrayIndexOutOfBoundsException`.
+Fixed in msgpack-jackson3; needs the same fix in msgpack-jackson.
+
+**Practical impact:** Low. `writeString(char[], offset, len)` is a low-level Jackson
+streaming API used by Jackson's own internals and performance-sensitive custom serializers,
+not by typical application code. Normal users call `writeString(String)` instead.
+
+## 9. `MessagePackGenerator`: `writeByteArrayTextValue` ASCII path ignores offset
+
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java:512`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
+
+`addValueNode(new AsciiCharString(text))` stores the entire backing array instead of
+the requested slice `[offset, offset+len)`. Callers such as `writeRawUTF8String` and
+`writeUTF8String` with non-zero offsets will serialize garbage bytes.
+Fixed in msgpack-jackson3 using `System.arraycopy`; needs the same fix in msgpack-jackson.
+
+**Practical impact:** Low. `writeUTF8String(byte[], offset, len)` is a low-level API
+called by Jackson's streaming infrastructure or custom serializers working with raw
+byte buffers. Typical application code goes through ObjectMapper, which always passes
+offset=0 for plain byte arrays.
+
+## 10. `MessagePackGenerator`: ByteBuffer serialization ignores `position()`
+
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java:369`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
+
+`writePayload(bb.array(), bb.arrayOffset(), len)` ignores `bb.position()`. For any
+ByteBuffer with a non-zero position (e.g. from `ByteBuffer.wrap(data, offset, len)` or
+after reads), this serializes bytes starting at the wrong offset.
+Fixed in msgpack-jackson3 using `bb.arrayOffset() + bb.position()`; needs the same fix in msgpack-jackson.
+
+**Practical impact:** Moderate. This is the most realistic end-user scenario: a POJO
+with a `ByteBuffer` field that was sliced or partially consumed will silently produce
+corrupt serialized output. No exception is thrown.
+