Stop an Arabic form's context walk at the edge of the run (#204)

[jakejackson1]

Closes #204.

What

Shaper\Arabic::glyphs() checks a joining form's backtrack (prel) and lookahead (postl) by walking over the glyphs the lookup ignores. Neither walk stopped at the edge of the run. Both now check isset($chars[...]) before they look the glyph up in the ignore pattern:

while (isset($chars[$i + $ig + $k]) && GlyphString::inList($arabGlyphs[$char]['ignore'][$retk], $chars[$i + $ig + $k])) {

The isset() check after the walk already marked a position the run doesn't have as not matched, so that is now the result whenever the walk runs out. skipTransparent() is the only other while over $chars in Arabic.php, and it was already bounded.

Reproduced first, on this base (#205's inList() in place)

The snippet from the issue, plus a lookahead version (BEH FATHA, where BEH's isolated form has a postl of DAL):

	backtrack	lookahead
PHP 8.5.4 at d9451ed	timeout 10 exits 124 (hangs)	exits 124 (hangs)
PHP 8.5.4 at 37f1e92 (before #205)	exits 124	exits 124
PHP 7.4 / 7.2 / 5.6 at d9451ed	returns, no match, Undefined offset: -1	returns, no match, Undefined offset: 2
every version after this change	returns, no match, no notices	same

inList() doesn't change the hang. It still looks for the glyph with strpos($glyphs, $glyph). With a null glyph, PHP 8 searches for "" and finds it at offset 0. strlen(null) is 0, and the character after offset 0 is the pattern's opening (, which isn't a hex digit, so inList() returns true on every pass. On PHP 7, null is searched for as "\0", which isn't found, so the loop stops. The result there was already right, because the isset() after the loop turns it into no match. PHP 7 only had the notices.

Why "no match" is correct

HarfBuzz 14.3.1, src/hb-ot-layout-gsubgpos.hh. match_lookahead() fails the rule as soon as the skippy iterator can't find the next position:

  for (unsigned int i = 0; i < count; i++)
  {
    unsigned unsafe_to;
    if (!skippy_iter.next (&unsafe_to))
    {
      *end_index = unsafe_to;
      return_trace (false);
    }
  }

The iterator's next() skips glyphs only while idx < stop, and returns false once it reaches the end of the buffer:

  bool next (unsigned *unsafe_to = nullptr)
  {
    auto *info = c->buffer->info;
    const signed stop = (signed) end - 1;
    while ((signed) idx < stop)
    {
      idx++;
      switch (match (info[idx]))
      {
	case MATCH: ...  return true;
	case NOT_MATCH: ... return false;
	case SKIP:
	  continue;
      }
    }
    if (unsafe_to)
      *unsafe_to = end;
    return false;
  }

match_backtrack() works the same way with skippy_iter.prev(), which stops at idx > 0 and returns false.

Tests

• ArabicTest::testAFormsContextWalkReadsNothingPastTheEdgeOfTheRun runs in-process with an error handler that turns every error into an ErrorException. That covers the PHP 7 notices, and it means a regression fails on the first notice, so this test can't hang. The data provider covers:

  • a backtrack that runs out at the start of the run
  • a lookahead that runs out at the end
  • a two-position lookahead whose second position runs out after the first matches
  • a backtrack matched past the ignored glyphs
  • a lookahead matched past the ignored glyphs

• ArabicTest::testAFormsContextWalkReturnsAtTheEdgeOfTheRun runs all the same cases in one child process (tests/Mpdf/Fixtures/arabic-shape.php), started with proc_open and bypass_shell the way ShapingTraceTest does it:

  • The child calls set_time_limit(5) on itself instead of relying on a timeout binary, so the limit also works on Windows. There it counts wall-clock time; on Unix it counts CPU time, and a runaway loop uses CPU.
  • It sets error_reporting(E_ERROR), so a walk that warns on every pass can't fill the stderr pipe and block. It still reports the fatal "Maximum execution time" error.
  • The case is passed as base64 JSON, because Windows argument quoting drops double quotes.

• ArabicContextEdgeTest is end to end through a real font with TextRecordingMpdf, using dir="rtl". Expected glyphs, in visual order:

  text	mPDF draws	hb-shape 14.3.1
  BEH BEH FATHA	064E 0628 0628	uni064E uni0628 uni0628
  FATHA BEH BEH	0628 0628 064E	uni0628 uni0628 uni064E
  BEH BEH FATHA LOW-ALEF	08AD 064E E002 0628	uni08AD uni064E uni066E.fina uni0628
  LOW-ALEF FATHA BEH BEH	0628 E004 064E 08AD	uni0628 uni066E.init uni064E uni08AD

The synthetic font

NotoSansArabic-ContextEdge-Synthetic.ttf starts from NotoSansArabic-Joining-Subset (Noto Sans Arabic 2.012, OFL 1.1) and was rebuilt with fontTools 4.59.2:

• adds a FATHA glyph uni064E at U+064E. It reuses dotbelowar's outline and advance and is in GDEF class 3.
• replaces GSUB with:
  • init: a Type 6 Format 3 lookup with IgnoreMarks. Backtrack is uni08AD, input is uni0628, and it applies uni0628 → uni066E.init.
  • fina: the same, with a lookahead of uni08AD, applying uni0628 → uni066E.fina.
  • medi: a single substitution.
• drops ccmp. mPDF works out joining after ccmp, from glyphs rather than characters, so a beh that ccmp splits into a rasm and a dot joins nothing. That is a separate bug, opened as A letter ccmp takes apart into a base and a mark joins nothing in the Arabic shaper #209.
• renames name IDs 1, 4 and 6.

The font puts prel/postl into its rtlSUB (see tests/data/fontcache/NotoSansArabic-ContextEdge-Synthetic.json). It is independent of #195's GSUB62Positions font, which is on a sibling branch.

Fixtures

I deleted tests/Mpdf/tmp/mpdf, tmp/mpdf and tmp/ttfontdata, then ran composer fontcache:update all, otldump:update all, shaping:update all and subset:update all. The only new files are the four fixtures for the new font, and no existing fixture changed. In the new shaping fixture, the Arabic run and the font's own characters come back unchanged. Only a beh with a low alef on the matching side takes a form, and none of those runs has one.

Mutation check

I removed each isset() in turn and ran --filter 'ContextWalk|ArabicContextEdge'. These runs were made before /simplify, when the child ran once per case; after batching, removing the lookahead bound still fails the single child test with the same time-limit fatal (5.1s).

• Backtrack bound removed (finished in 6s):
  • The child-process test failed on "a backtrack that runs out at the start of the run" with Fatal error: Maximum execution time of 5 seconds exceeded.
  • The in-process test errored with ErrorException: Undefined array key -1.
  • ArabicContextEdgeTest "a word starting with a mark" errored with the same message.
• Lookahead bound removed (finished in 11s):
  • The child-process test failed on both lookahead cases with the time-limit fatal.
  • The in-process test errored with Undefined array key 2 and 4.
  • ArabicContextEdgeTest "a word ending in a mark" errored with Undefined array key 3.

Nothing hung.

Verification

• vendor/bin/phpunit: 2095 tests, 5983 assertions, 1 skipped (base: 2078 / 5955 / 1)
• --group=snapshot: 79 tests, 110 assertions
• composer cs: clean
• phpstan: 33 errors, the same as the base, none in Arabic.php

Stack

Base is fix/193-glyph-class-membership (#205), still open with head d9451ed. #186 and #188 further down have merged, and #190 already targets gravitypdf. #205 hasn't moved, so this branch didn't need a rebase.

/simplify

• Applied: the child-process test ran PHP once per case, and now runs every case in one child. The source comment is shorter.
• Skipped:
  • Merging the prel/postl walks into one direction-parameterised loop. The behaviour would be the same, but the diff would be wider and the upstream change harder to mirror.
  • A shared proc_open helper with ShapingTraceTest. There are only two uses.
  • Dropping the child-process test as redundant with the error-handler test. It is the guard against a regression that reads past the edge without raising a notice, and the task asked for it.
  • Making GlyphString::inList() refuse a null or empty glyph. That is outside Arabic.php and would hide the out-of-range read rather than bound it.

Upstream mirror

mpdf/mpdf development (744f75a) has the same two loops in Otl::get_arab_glyphs() (src/Otl.php:2903), which arabic_shaper() (:2842) calls. There they use strpos() directly, and on PHP 8 strpos($ignore, null) also finds "":

• src/Otl.php:2957: while (strpos($this->arabGlyphs[$char]['ignore'][$retk], $chars[$i - $ig - $k]) !== false) becomes while (isset($chars[$i - $ig - $k]) && strpos(...) !== false)
• src/Otl.php:2974: the same, with $chars[$i + $ig + $k]

The upstream PR needs a CHANGELOG.md entry; this one doesn't have one.

🤖 Generated with Claude Code

[jakejackson1]

Rebased: #190 was squash-merged into gravitypdf (f9b3c0e), so this stack still carried its pre-squash commits. This branch is rebased onto its rebased parent with --onto; no conflicts and the base is unchanged. Suite, snapshots and cs pass on the new head, with no fixture movement.

[jakejackson1]

This PR merged into a base branch that had already been squash-merged, so its change never reached gravitypdf. It's carried to gravitypdf by #219.