Fix DataReader.findNewLine with multiple lone EOL character (helidon-io#9362)

When there is multiple lone CR within a buffer, the virtual index is accumulated each time with the node index.
The virtual index may be incorrectly seen as >= max, which ignores CRLN within max.

- Fix virtual index accumulation
- Re-use the branch that changes the buffer
- Renamed indexWithinNode -> fromIndexNode
- Renamed crIndex -> crIndexNode
- Renamed lfIndex -> lfIndexNode
- Add unit tests

Author: romain-grecourt

common/buffers/src/main/java/io/helidon/common/buffers/DataReader.java

@@ -395,69 +395,62 @@ public String debugDataHex() {
      */
     public int findNewLine(int max) throws IncorrectNewLineException {
         ensureAvailable();
-        int idx = 0;
         Node n = head;
-        int indexWithinNode = n.position;
+        int idx = 0;
+        int fromIndexNode = n.position;
 
         while (true) {
             byte[] barr = n.bytes;
-            int maxLength = Math.min(max - idx, barr.length - indexWithinNode);
-            int crIndex = Bytes.firstIndexOf(barr, indexWithinNode, indexWithinNode + maxLength, Bytes.CR_BYTE);
+            int maxLength = Math.min(max - idx, barr.length - fromIndexNode);
+            int crIndexNode = Bytes.firstIndexOf(barr, fromIndexNode, fromIndexNode + maxLength, Bytes.CR_BYTE);
 
-            if (crIndex == -1) {
-                int lfIndex = Bytes.firstIndexOf(barr, indexWithinNode, indexWithinNode + maxLength, Bytes.LF_BYTE);
-                if (lfIndex != -1) {
+            if (crIndexNode == -1) {
+                int lfIndexNode = Bytes.firstIndexOf(barr, fromIndexNode, fromIndexNode + maxLength, Bytes.LF_BYTE);
+                if (lfIndexNode != -1) {
                     if (!ignoreLoneEol) {
-                        throw new IncorrectNewLineException("Found LF (" + (idx + lfIndex - n.position)
+                        throw new IncorrectNewLineException("Found LF (" + (idx + lfIndexNode - n.position)
                                                                     + ") without preceding CR. :\n" + this.debugDataHex());
                     }
                 }
-                // not found, continue with next buffer
-                idx += maxLength;
-                if (idx >= max) {
-                    // not found and reached the limit
-                    return max;
-                }
-                n = n.next();
-                indexWithinNode = n.position;
-                continue;
             } else {
                 // found, next byte should be LF
-                if (crIndex == barr.length - 1) {
+                if (crIndexNode == barr.length - 1) {
                     // found CR as the last byte of the current node, peek next node
                     byte nextByte = n.next().peek();
                     if (nextByte == Bytes.LF_BYTE) {
-                        return idx + crIndex - n.position;
+                        return idx + crIndexNode - fromIndexNode;
                     }
                     if (!ignoreLoneEol) {
-                        throw new IncorrectNewLineException("Found CR (" + (idx + crIndex - n.position)
+                        throw new IncorrectNewLineException("Found CR (" + (idx + crIndexNode - n.position)
                                                                     + ") without following LF. :\n" + this.debugDataHex());
                     }
                 } else {
                     // found CR within the current array
-                    byte nextByte = barr[crIndex + 1];
+                    byte nextByte = barr[crIndexNode + 1];
                     if (nextByte == Bytes.LF_BYTE) {
-                        return idx + crIndex - n.position;
+                        return idx + crIndexNode - fromIndexNode;
                     }
                     if (!ignoreLoneEol) {
                         throw new IncorrectNewLineException("Found CR (" + idx
                                                                     + ") without following LF. :\n" + this.debugDataHex());
                     }
-                    indexWithinNode = crIndex + 1;
-                    idx += indexWithinNode;
+                    idx += (crIndexNode - fromIndexNode + 1);
+                    fromIndexNode = crIndexNode + 1;
                     if (idx >= max) {
                         return max;
                     }
                     continue;
                 }
             }
 
+            // not found, continue with next buffer
             idx += maxLength;
             if (idx >= max) {
+                // not found and reached the limit
                 return max;
             }
             n = n.next();
-            indexWithinNode = n.position;
+            fromIndexNode = n.position;
         }
     }
 

common/buffers/src/test/java/io/helidon/common/buffers/DataReaderTest.java

@@ -16,6 +16,7 @@
 
 package io.helidon.common.buffers;
 
+import java.nio.charset.StandardCharsets;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.jupiter.api.Test;
@@ -30,7 +31,7 @@ void testFindNewLineWithLoneCR() {
         // reading N bytes at a time until a new line is found
         // with data containing a lone CR
 
-        byte[] data = new byte[] {0, 0, (byte) '\r', 0, (byte) '\r', (byte) '\n'};
+        byte[] data = "00\r0\r\n".getBytes(StandardCharsets.US_ASCII);
         AtomicReference<byte[]> ref = new AtomicReference<>(data);
         DataReader dataReader = new DataReader(() -> ref.getAndSet(null), true);
 
@@ -41,4 +42,32 @@ void testFindNewLineWithLoneCR() {
         dataReader.skip(n);
         assertThat(dataReader.findNewLine(n), is(0));
     }
+
+    @Test
+    void testFindNewLineWithMultipleLoneCR() {
+        // if the stream index is accumulated with the node index for each lone CR
+        // it may exceed max and the new line is ignored
+
+        byte[] data = "00\r\r\r\n".getBytes(StandardCharsets.US_ASCII);
+        AtomicReference<byte[]> ref = new AtomicReference<>(data);
+        DataReader dataReader = new DataReader(() -> ref.getAndSet(null), true);
+
+        int n = 5;
+        assertThat(dataReader.findNewLine(n), is(4));
+    }
+
+    @Test
+    void testFindNewLineWithMultipleLoneWithinMax() {
+        // if the stream index is not updated for each lone CR
+        // the computed search range is too big and a value greater than max is returned
+
+        byte[] data = "00\r00\r\n00".getBytes(StandardCharsets.US_ASCII);
+        AtomicReference<byte[]> ref = new AtomicReference<>(data);
+        DataReader dataReader = new DataReader(() -> ref.getAndSet(null), true);
+
+        int n = 4;
+        assertThat(dataReader.findNewLine(n), is(n));
+        dataReader.skip(n);
+        assertThat(dataReader.findNewLine(n), is(1));
+    }
 }