AnalyzingSuggester with payloads checks the wrong bytes for the reserved U+001F, so a valid BytesRef slice is rejected

When AnalyzingSuggester (and FuzzySuggester, which inherits build()) is built with payloads, it stores each suggestion as surface form + U+001F + payload and splits at the first U+001F at lookup, so it rejects surface forms that contain that byte. The check reads surfaceForm.bytes[i] for i in [0, length) and never adds surfaceForm.offset (AnalyzingSuggester.java:466-467 on main 645d4fe14a20), while right after the loop the method writes the real slice [offset, offset + length) (:473). For a BytesRef with offset > 0 it therefore checks bytes that are not the surface form. The valid "ab" at offset 1 of {0x1F, 'a', 'b'} is rejected with IllegalArgumentException; a slice that really contains U+001F is accepted when the first length bytes of the array do not, and comes back with a truncated key and a corrupted payload, or, with the default EXACT_FIRST, is dropped (-da) or trips an assertion (-ea). The trigger is an application InputIterator that returns slices of a larger buffer, for example values from a BytesRefHash or a MemoryIndex TermsEnum, where one 31-byte value stored first puts its length prefix 0x1F at byte 0 and fails the whole build. Lucene's own payload-capable iterators return offset-0 refs, and builds without payloads never run the check. The check came with payloads in LUCENE-4820 (2013, first released in 4.3.0); it fails on all 22 releases executed, from 4.3.0 to 10.6.0, and on main, and every other release tag from 4.3.0 on has the same line. The fix is one line: surfaceForm.bytes[surfaceForm.offset + i].

• Separator. private static final int PAYLOAD_SEP = '\u001f'; (:161). With payloads, the FST output of an entry is surface form + PAYLOAD_SEP + payload (:579-586), and getLookupResult splits it at the first PAYLOAD_SEP (:623-640, scanning with output2.offset at :628).
• Input. build(InputIterator) (:389) sets hasPayloads = iterator.hasPayloads() (:394) and reads each surface form with iterator.next() (:415). It analyses the slice through toAutomaton (:417), which calls surfaceForm.utf8ToString() (:868) and honours the offset.
• The check, without the offset (:465-474):

if (hasPayloads) {
  for (int i = 0; i < surfaceForm.length; i++) {
    if (surfaceForm.bytes[i] == PAYLOAD_SEP) {                 // :467  reads bytes[0, length)
      throw new IllegalArgumentException(
          "surface form cannot contain unit separator character U+001F; this character is reserved");
    }
  }
  output.writeShort((short) surfaceForm.length);
  output.writeBytes(surfaceForm.bytes, surfaceForm.offset, surfaceForm.length);   // :473  writes the real slice
  output.writeBytes(payload.bytes, payload.offset, payload.length);

• Where it runs. Inside the loop over the entry's analyzed forms (:419), so once per analyzed form of the entry.
• No out-of-bounds read. For a valid BytesRef, offset + length <= bytes.length, so the bytes read are inside the array; they are only compared with 0x1F, never copied or returned. The only effect is whether build() throws.
• FuzzySuggester extends AnalyzingSuggester (FuzzySuggester.java:66) without overriding build, so it fails the same way.

The accepted entry is stored as a<U+001F>b<U+001F>p and split at the first U+001F. Without EXACT_FIRST, lookup("a") returns key a with payload b<U+001F>p. With the default EXACT_FIRST, the entry is missing (-da) or the lookup throws AssertionError at :808 (-ea).

BytesRefHash, which MemoryIndex uses for its terms, returns slices of a shared block: BytesRefBlockPool.fillBytesRef sets term.offset = pos + 1 after a 1-byte length prefix (BytesRefBlockPool.java:62-75, written at :101-103). If the first value in a block is 31 bytes long, byte 0 of the block is 31 = 0x1F, and the check reads byte 0 for every non-empty value from that block: the build fails at the first such value although no value contains 0x1F. Executed with a wrapped MemoryIndex TermsEnum and with BytesRefHash.get; the compact copies build.

• Who is hit. Applications that build a payload-enabled AnalyzingSuggester or FuzzySuggester (both @lucene.experimental, AnalyzingSuggester.java:98, FuzzySuggester.java:64) from their own InputIterator that returns BytesRef slices with offset > 0: records in one shared buffer, values from a BytesRefHash, a wrapped MemoryIndex TermsEnum.
• Default vs opt-in. Opt-in twice: payloads must be on, and the iterator must hand out slices. Every Lucene-provided payload iterator (DocumentDictionary, FileDictionary, BufferedInputIterator, SortedInputIterator, UnsortedInputIterator) returns offset-0 refs, and builds without payloads never run the check. A read-only code search of the default branches (not exhaustive) found that the dictionaries Solr configures are Lucene's own, and found no production use of these suggesters in Elasticsearch or OpenSearch (only test method names).
• What goes wrong. Mostly a loud failure: a slice that contains no U+001F is rejected whenever a 0x1F byte sits in bytes[0, length) outside the slice, and the suggester cannot be built or rebuilt while that value is in the input. Secondary: a slice that does contain U+001F, input the API means to reject, is accepted when bytes[0, length) holds no 0x1F and comes back with a truncated key and a corrupted payload, or is dropped (-da) or trips an assertion (-ea) under EXACT_FIRST.
• Scale. None needed: one entry is enough. Deterministic, assertions on or off.
• Workaround. Return BytesRef.deepCopyOf(ref) from the iterator; the compact copies of valid entries used as controls in the finder's, the independent reproducer's, the devil's advocate's, the security probe's and the history repros build correctly (the minimal test has no such control).
• How likely. Rare: it needs payloads plus a slice-returning application iterator, and no matching public report was found in the recorded searches.

Copy the test into lucene/suggest/src/test/org/apache/lucene/search/suggest/analyzing/ of a Lucene checkout and run it with gradle (command below). It fails inside build(), before the lookup. With the one-line fix it passes unchanged, so it doubles as the regression test.

The seed drives MockAnalyzer and the test framework's other randomization, such as locale, time zone and whether assertions are on (DEADBEEF alone runs with tests.asserts=false; add -Ptests.asserts=true to force them on); the check depends on none of these: the test fails with two seeds and with assertions on and off. Executed: gradle on a fresh 645d4fe14a20 with assertions off and on (fails); the standalone runner on the stock main jars and on the 10.6.0 release jars, each with -ea and -da (fails, same exception).

cp $REPRO/TestAnalyzingSuggesterPayloadSurfaceSlice.java $LUCENE/lucene/suggest/src/test/org/apache/lucene/search/suggest/analyzing/
cd $LUCENE && ./gradlew :lucene:suggest:test --tests org.apache.lucene.search.suggest.analyzing.TestAnalyzingSuggesterPayloadSurfaceSlice -Ptests.seed=DEADBEEF
# assertions forced on: add -Ptests.asserts=true (same failure)

# gradle, main 645d4fe14a20, -Ptests.seed=DEADBEEF (as drawn: tests.asserts=false; the same with -Ptests.asserts=true)
TestAnalyzingSuggesterPayloadSurfaceSlice > testSurfaceFormSliceWithPayload FAILED
    java.lang.AssertionError: expected surface form "ab" to be accepted, but build rejected it
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:D57D816ED23036C6]:0)
        at org.apache.lucene.search.suggest.analyzing.TestAnalyzingSuggesterPayloadSurfaceSlice.testSurfaceFormSliceWithPayload(TestAnalyzingSuggesterPayloadSurfaceSlice.java:75)

        Caused by:
        java.lang.IllegalArgumentException: surface form cannot contain unit separator character U+001F; this character is reserved
            at org.apache.lucene.search.suggest.analyzing.AnalyzingSuggester.build(AnalyzingSuggester.java:468)
            at org.apache.lucene.search.suggest.analyzing.TestAnalyzingSuggesterPayloadSurfaceSlice.testSurfaceFormSliceWithPayload(TestAnalyzingSuggesterPayloadSurfaceSlice.java:73)

1 test completed, 1 failed

# standalone runner, seed DEADBEEF
main 645d4fe14a20  -ea / -da   tests_run=1 failures=1   same exception
10.6.0             -ea / -da   tests_run=1 failures=1   same exception

• Contract. BytesRef.java:22: "Represents byte[], as a slice (offset + length) into an existing byte[]". InputIterator (InputIterator.java:29-31) and BytesRefIterator.next() (BytesRefIterator.java:24-34: "The returned BytesRef may be re-used across calls to next") state no offset-0 precondition; nor do Lookup.build and the AnalyzingSuggester javadoc. The IAE text states the rule for the surface form, and the rest of the class treats the slice as the surface form: the write at :473, toAutomaton (:868), the lookup split (:628) and sameSurfaceForm (:650-665) all use the offset.
• Tests and practice. LuceneTestCase's newBytesRef(byte[], int, int) exists to "tickle latent bugs that fail to look at BytesRef.offset" (LuceneTestCaseParent.java:1209-1213). No suggest test uses newBytesRef, and none asserts the build-time U+001F message. Fixes of the same shape exist: LUCENE-10052 (FSTTermsReader.IntersectEnum "was illegally ignoring BytesRef.offset") and #9017 / LUCENE-7968, a slice bug in this class's AnalyzingComparator, fixed in 2017.
• History. The check, PAYLOAD_SEP and the payload API came in one commit, 234fa47dba43 (LUCENE-4820, #5885, 2013). That commit's lookup-side split uses output2.offset, and the offset-aware writeBytes predates it (LUCENE-3842). The issue thread discusses how the payload is packed, never offsets or slices.

The strongest case for "works as designed": the class is @lucene.experimental, U+001F is deliberately reserved, and every Lucene-provided payload iterator returns offset-0 refs, so the code could be said to assume compact refs.

Why it fails: nothing documents that assumption; InputIterator is a public interface for applications to implement; the same method uses the offset right after the loop (:473); and the project treats ignoring BytesRef.offset as a bug (the newBytesRef javadoc, LUCENE-10052, LUCENE-7968). @lucene.experimental allows API changes, not wrong validation, and bugs in this class have been fixed before (LUCENE-5120, LUCENE-7968). These points limit reach, not validity. They limit it a lot: the usual symptom is a loud exception at build time, and no production caller with a slice-returning iterator was identified, hence priority low. It still counts as a bug rather than noise, because valid input is rejected through the public API with assertions off, and BytesRefHash / MemoryIndex are a natural source of such slices.

One code fix, validated with gradle in a fresh scratch worktree at 645d4fe14a20, and an application-side workaround for released versions.

In AnalyzingSuggester.build, read the slice's bytes, as the write right after the loop (:473) already does.

if (hasPayloads) {
  for (int i = 0; i < surfaceForm.length; i++) {
    if (surfaceForm.bytes[surfaceForm.offset + i] == PAYLOAD_SEP) { // was: surfaceForm.bytes[i]
      throw new IllegalArgumentException(
          "surface form cannot contain unit separator character U+001F; this character is reserved");
    }
  }

Trade-offs

• No change for offset-0 refs: the expression is the same.
• Slices that really contain U+001F are now rejected, as their compact copies already are; that is the rule the exception states.
• Backportable to 10.x; FuzzySuggester is fixed with it.

What the checks validated

• Gradle, fresh scratch worktree at 645d4fe14a20, JDK 25, -Ptests.seed=DEADBEEF: without the fix the minimal test fails, with assertions off (as drawn) and forced on. With the fix it passes unchanged, together with TestAnalyzingSuggester (23 tests) and TestFuzzySuggester (14): 38 of 38, assertions off and on. The whole :lucene:suggest:test passes (32 suites, 237 tests, 7 skipped, 0 failures), and so does :lucene:suggest:checkGoogleJavaFormat. Earlier runs of the same steps, one by an independent fact check, gave the same counts.
• Shadow runs of the same line: the finder's 2 tests, an independent reproducer's 6 (both suggesters), the MemoryIndex test and the security probe's 4 pass; the 4.3.0 history port passes with it.

Have the application's InputIterator.next() return BytesRef.deepCopyOf(ref) instead of the slice.

Trade-offs

No Lucene change; one allocation and copy per entry. A workaround, not a fix.

What the checks validated

The compact copies of valid entries used as controls in the finder's, the independent reproducer's, the devil's advocate's, the security probe's and the history repros build correctly.

Method: a bytecode def-use scan plus a bounded metamorphic harness (the hunt's semi-formal tier), in a hunt unit on BytesRef offset/length handling, run on the stock main jars at ea57360912ed.

• Scan. A data-flow pass over the bytecode of every non-sandbox module flags array reads whose index ignores BytesRef.offset on a value returned by an iterator. A mutation probe checked it: all 7 planted defective methods are flagged, and of 4 correct methods none gets an offset flag. It flagged AnalyzingSuggester.build at line 467.
• Harness. Oracle: a suggester built from slices must behave exactly as one built from compact copies of the same bytes. Slices at offsets 1 and 7, with fill bytes 0x00, 0xFF, 0x1F, a and a self-copy. Result: AnalyzingSuggester and FuzzySuggester with payloads mismatch in 87 of 310 cases each; AnalyzingSuggester without payloads, FSTCompletionLookup, WFSTCompletionLookup, TSTLookup and AnalyzingInfixSuggester show 0. Planted offset-ignoring mutants made the harness report mismatches, as expected.
• The finding was then reduced to a public-API JUnit test, later to the single test shown here. Bounded scope only; no model checker, SMT solver or prover was involved.