[alek-prykhodko]

Summary

The StuffItX English preprocessor replaces common words with their position in a shared
100366-word dictionary. That position is spelled out in the stream as base 52 digits,
using letters as the digits — a to z for 1 to 26, A to Z for 27 to 52 — most
significant first, terminated by the first non-letter.

produceByteAtOffset: accumulates the number in an int with no bound on how many
digits it will read. Three digits cover every word in the dictionary, but nothing stops
a malformed stream from supplying more: seven letters accumulate to 20158268677, which
overflows a signed 32-bit int and wraps negative.

The only bounds check ran after the loop, and asks whether the number is too large:

if(index>=NumberOfWords) [XADException raiseIllegalDataException];

A negative index is not too large, so it passes. The lookup that follows then reads in
front of the table:

wordlen=pointers[index+1]-pointers[index]-1;
memcpy(wordbuf,pointers[index],wordlen);

Unlike the data race in #NNN this needs no concurrency at all — one archive carrying
seven letters after a word marker reproduces it every time. Feeding the decoder a
wordcode followed by aaaaaaa wraps the index to -1316567803 and faults:

  Exception Type:  EXC_BAD_ACCESS (KERN_PROTECTION_FAILURE)
  Exception Codes: code=2, address=0x952362830

  Thread 1:
    0  XADMaster    -[XADStuffItXEnglishHandle produceByteAtOffset:]
    1  XADMaster    -[XADByteStreamHandle streamAtMost:toBuffer:]
    2  XADMaster    -[XADStreamHandle readAtMost:toBuffer:]

A wrapped index that happens to land back inside the table is worse than the crash in
one respect: it decodes a word that was never asked for and corrupts the output with no
error at all.

Key Changes

• Check the bound on every digit instead of once after the loop. Digits are always
  positive, so a number that has already passed the word count can only grow from there
  — bailing out immediately is equivalent, minus the overflow. With the check in place
  index never exceeds 100365, so the next multiplication reaches about 5.2 million at
  most and can no longer approach INT_MAX.
• Document the base 52 encoding next to the check. Neither the letters nor a comparison
  against the word count read as anything obvious on their own.
• Add a test feeding a stream with seven letters after a word marker. Without the fix it
  takes the test runner down with EXC_BAD_ACCESS; with it, XADIllegalDataError is
  raised and caught.

[Copilot]

Pull request overview

This PR hardens the StuffItX English preprocessor decoder against malformed “word number” sequences that could overflow a 32-bit int, wrap negative, and lead to out-of-bounds dictionary pointer access.

Changes:

• Adds an in-loop bounds check during base-52 word-number accumulation to prevent integer overflow and negative wraparound.
• Documents the base-52 letter encoding adjacent to the decoding logic for maintainability.
• Adds a regression test that feeds an overlong word number and asserts XADIllegalDataError is raised (preventing hard crashes).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
XADStuffItXEnglishHandle.m	Moves the word-index bounds check into the digit-accumulation loop to prevent overflow/negative wrap and OOB reads.
XADMasterTests/Stuffit/XADStuffItXEnglishHandleTests.m	Adds a regression test ensuring overlong word-number sequences raise an XADIllegalDataError instead of crashing.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[PaulTaykalo]

Good fix. The bound had to move inside the base-52 digit loop — the old post-loop index>=NumberOfWords check couldn't catch an index that had already overflowed a signed int and wrapped negative, which is what let the dictionary lookup read in front of the table. Checking on every digit rejects an overlong word number before it can overflow. The regression test for the overlong-run case is a nice include. LGTM.