Fix advancing invalid UTF characters - #945

I admit that I find the JIT code hard to follow (although I am getting more familiar with it).

GPT explained it to me, and the change made sense. I couldn't find any bugs either.

Great!

GPT says....

Verdict: the fix is logically correct, and I do not see a blocking correctness issue. It restores the essential bump-along invariant:

After testing position p, advance to the end of the valid UTF character beginning at p; if no valid character begins there, advance exactly one code unit.

The old code instead skipped continuation-looking code units after p, regardless of whether they belonged to a valid character beginning at p. That could skip legitimate match starting positions. The new implementation validates from the current position and consumes additional code units only when they form one valid UTF character.

PR and bug context

PR #945 — Fix advancing invalid UTF characters addresses issue #944 — JIT matching issue with lookbehinds and invalid UTF matching.

The issue’s reproducer is approximately:

/(?<=\K.)/utf,match_invalid_utf,allow_lookaround_bsk
    x\x80\x80
    x\x80\x80y

At offset zero, the lookbehind cannot succeed because there is no preceding character. The next match attempt must therefore be at the offset immediately after x. There, the lookbehind can consume x.

The old JIT bump-along loop advanced over x, then also skipped both following 0x80 bytes because they looked like UTF-8 continuation bytes. It jumped directly from offset 0 to offset 3 and never tried offset 1.

That matches the diagnosis in the issue discussion: the bug is in the JIT’s unanchored main-loop advancement optimization, not lookbehind itself.

Why the old logic was wrong

The old UTF-8 branch effectively did this:

 STR_PTR++;                         // consume the current byte

-if (invalid_utf) {
-    while (STR_PTR < STR_END &&
-           is_continuation_byte(*STR_PTR))
-        STR_PTR++;
-}

That is safe only if the byte at the original position is known to be the start of a valid UTF character. Under PCRE2_MATCH_INVALID_UTF, it is not.

For example:

Subject bytes:  78 80 80
                x  .  .

At p=0:
  Correct next position: p=1
  Old next position:     p=3

The bytes after x are continuation-shaped, but they do not belong to x. Their bit pattern alone is not enough to justify skipping them.

The same conceptual problem existed in UTF-16: after any current code unit, the old loop skipped following low surrogates without first establishing that the current code unit was a high surrogate forming a valid pair.

What the new logic does

The new UTF-8 code reads the current byte before incrementing the pointer. After the mandatory one-byte increment:

• ASCII and standalone continuation bytes (< 0xc0) consume exactly one byte.
• A possible leading byte is passed to the existing invalid-aware decoder.
• If decoding succeeds, the decoder leaves STR_PTR after the full valid character.
• If decoding fails, the caller restores STR_PTR to the saved one-byte-advanced position.

-if (common->utf && !common->invalid_utf) readuchar = TRUE;
+if (common->utf) readuchar = TRUE;

 if (common->invalid_utf)
   {
-  /* Blindly skip following continuation bytes. */
-  while (STR_PTR < STR_END && next_byte_is_continuation)
-    STR_PTR++;
+  jump = CMP(SLJIT_LESS, TMP1, 0, SLJIT_IMM, 0xc0);
+  OP1(SLJIT_MOV, TMP3, 0, STR_PTR, 0);
+  add_jump(compiler, &common->utfreadchar_invalid,
+    JUMP(SLJIT_FAST_CALL));
+  OP2U(SLJIT_SUB | SLJIT_SET_Z, TMP1, 0,
+    SLJIT_IMM, INVALID_UTF_CHAR);
+  SELECT(SLJIT_EQUAL, STR_PTR, TMP3, 0, STR_PTR);
   JUMPHERE(jump);
   }

Here, TMP3 is the fallback position p + 1. The resulting rule is exactly:

valid UTF-8 sequence at p  => p + sequence_length
invalid sequence at p      => p + 1
ASCII at p                 => p + 1
continuation byte at p     => p + 1

That is the correct rule for enumerating possible match starts without entering the middle of a valid character and without hiding offsets inside malformed input.

UTF-16 analysis

The UTF-16 path follows the same rule:

 if (common->invalid_utf)
   {
-  /* Blindly skip following low surrogates. */
-  while (next_code_unit_is_low_surrogate)
-    STR_PTR++;
+  OP2(SLJIT_SUB, TMP2, 0, TMP1, 0, SLJIT_IMM, 0xd800);
+  jump = CMP(SLJIT_GREATER_EQUAL, TMP2, 0,
+    SLJIT_IMM, 0xe000 - 0xd800);
+  add_jump(compiler, &common->utfreadchar_invalid,
+    JUMP(SLJIT_FAST_CALL));
   JUMPHERE(jump);
   }

The helper’s contract is strengthened so that invalid input does not consume the second code unit:

-/* STR_PTR is undefined for invalid characters. */
+/* STR_PTR is unchanged for invalid characters. */

-exit_invalid[0] =
-  CMP(SLJIT_GREATER_EQUAL, TMP1, 0, SLJIT_IMM, 0xdc00);
+exit_invalid[0] =
+  CMP(SLJIT_GREATER_EQUAL, TMP2, 0, SLJIT_IMM, 0x400);

 ...
 exit_invalid[2] =
   CMP(SLJIT_GREATER_EQUAL, TMP1, 0, SLJIT_IMM, 0x400);

+JUMPHERE(exit_invalid[2]);
+OP2(SLJIT_SUB, STR_PTR, 0, STR_PTR, 0,
+  SLJIT_IMM, IN_UCHARS(1));
 JUMPHERE(exit_invalid[0]);
 JUMPHERE(exit_invalid[1]);

Walking the relevant cases:

The pointer rollback is correctly limited to the path where the helper has already loaded and tentatively consumed a second code unit. The “first unit is not a high surrogate” and end-of-buffer exits happen before that increment, so they need no rollback.

Why reusing do_utfreadchar_invalid() is appropriate

This is better than repairing the old continuation-skipping loop because the existing decoder already knows the complete validity rules:

• valid leading-byte ranges;
• required number and form of continuation bytes;
• overlong encodings;
• UTF-8 surrogate encodings;
• code points above U+10FFFF;
• truncated sequences;
• UTF-16 high/low surrogate pairing.

The bump loop should not maintain a weaker, approximate copy of those rules. Reusing the decoder prevents “looks like a continuation unit” from being confused with “belongs to the character at the current position.”

The UTF-8 helper previously documented STR_PTR as undefined on invalid input, but the new caller does not rely on its value: it restores the saved p + 1 position whenever the return value is INVALID_UTF_CHAR.

Tests

The added tests exercise both important manifestations of the bug.

Skipped match position after a valid character

/(?<=(.))/utf,match_invalid_utf
    x\x80
    \x{10000}\x80

Expected captures prove that the matcher attempts the position immediately after the valid character:

• after ASCII x;
• after a valid multi-unit U+10000.

Advancing past valid and invalid multibyte-looking input

/\b./utf,match_invalid_utf
    \x{10000}s
    \xf0s

This distinguishes:

• a valid four-byte UTF-8 character, which must be consumed as one character;
• an invalid/truncated leading byte, which must consume only that byte, leaving s as the next candidate.

The UTF-16 versions similarly cover:

• a valid surrogate pair;
• standalone high and low surrogates;
• a low surrogate following a valid pair.

The 32-bit expected-output update is sensible even though its bump logic did not require modification: every 32-bit code unit already corresponds to one candidate unit, so the new cross-width tests establish the expected behavior there too.

Residual concerns

I found no correctness defect in the changed machine-code generation. In particular:

• TMP1 is now initialized whenever UTF advancement needs it.
• The ASCII/continuation fast path deliberately bypasses decoding and consumes one byte.
• UTF-8 decoder failures cannot leak their temporary pointer movement.
• UTF-16 second-unit consumption is rolled back only when required.
• Valid UTF behavior remains equivalent to the old optimized behavior.
• UTF-32 behavior remains unchanged.
• The PR is currently mergeable with a clean merge state.
• There are no review or inline comments yet.
• GitHub reports all 39 check runs as completed, although the available API response did not expose their individual conclusions, so I cannot independently claim that all 39 passed.

Risk is therefore low-to-medium: low in scope and concept, but slightly elevated because this is generated JIT control flow across multiple architectures.

One worthwhile test improvement

The new tests demonstrate the bug’s mechanism, but they do not preserve the exact reporter reproducer involving \K.

• In testdata/testinput10 near the newly added lookbehind regression, consider also adding the exact issue JIT matching issue with lookbehinds and invalid UTF matching #944 pattern /(?<=\K.)/utf,match_invalid_utf,allow_lookaround_bsk with x\x80\x80 and x\x80\x80y. The generalized (?<=(.)) test is technically sufficient to catch the skipped bump-along position, but retaining the original reproducer would protect the full user-visible interaction between invalid UTF matching, lookbehind, and \K.

Merge readiness

Assessment: correct and ready to merge, assuming the completed CI checks have successful conclusions.

I would approve this implementation. The key reason is not merely that the new outputs look right: the implementation establishes the proper forward-progress invariant for every input position, while the old implementation made an invalid inference from the following code units.

Want me to…

1. Trace the exact generated-register state for every UTF-8 and UTF-16 malformed-sequence category
2. Draft a concise approval review summarizing the correctness argument
3. Design a more exhaustive regression-test matrix for invalid UTF bump-along behavior

This will be a small performance drop in the, but it seems unavoidable.