trailofbits / trailofbits/polyfile

Text tests run against the file's bytes for every non-UCS encoding, where libmagic uses its decoded buffer

Open
#3,551 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
390
Forks
31
Avg merge
7h 52m
Merged PRs (30d)
72

Description

Summary

MatchContext.text_test_context re-encodes the buffer for UCS encodings only, so PolyFile's text
pass runs against the file's bytes for every other text encoding. libmagic runs its text tests
against the UTF-8 rendering of its decoded ubuf regardless of which encoding it detected.

#3489 introduced text_test_context and fixed the UCS case, which was the one the corpus exercised.
This issue records the remainder.

The assumption that no longer holds

polyfile/magic.py:841-859:

def text_test_context(self, encoding: Optional[str]) -> "MatchContext":
    """The buffer libmagic runs its text tests against.

    libmagic decodes its input into a UCS-4 buffer, drops the byte order mark, re-encodes that
    buffer as UTF-8, and runs its text tests against the result rather than against the file's
    bytes (``file_ascmagic_with_encoding`` in ``file/src/ascmagic.c``). That only changes the
    bytes for a UCS encoding, so every other input keeps this context and the offsets its tests
    report stay offsets into the file.
    """
    if encoding is None or encoding not in _UCS_BOM_LENGTHS:
        return self

The sentence "that only changes the bytes for a UCS encoding" is the part that is wrong. It holds
for ASCII and UTF-8, where the decoded buffer is the file's bytes. It does not hold for:

  • The eight-bit families. A byte at or above 0x80 in ISO-8859 text is one byte in the file and
    two bytes in UTF-8, so both the bytes a text test sees and the offsets it reports differ.
  • EBCDIC, once #3507 lands. There the whole buffer is translated through a 256-byte table, so
    essentially every byte differs.

Impact

A text definition that matches high-byte Latin-1 or EBCDIC content can fail to match, and a test
that does match reports an offset into a buffer that is not the file.

No corpus stem covers it. file/tests/ has no high-byte Latin-1 or EBCDIC stem whose expected
result depends on a text test, which is why #3489 could fix UCS alone and stay green.

The tradeoff, which is why the current behavior is defensible

Returning self keeps reported offsets meaningful as offsets into the file. Re-encoding makes the
bytes faithful to libmagic but makes every offset an offset into a derived buffer, which is a
different thing from what PolyFile reports everywhere else and what its Match objects mean.

libmagic does not face this, because it does not report structure — it prints a description and
stops. PolyFile maps files, so it has a constraint libmagic does not. Resolving this means deciding
whether text-test offsets are file offsets or decoded-buffer offsets, and possibly mapping between
them, rather than just widening the condition on line 857.

Suggested next step

Establish first whether any bundled text definition actually matches content above 0x7F. If none
does, this is latent and the fix is a documentation correction plus a test that pins the intent. If
some do, the offset question above has to be answered before the buffer is switched.

Found while implementing #3507. Related: #3489, which fixed the UCS half, and #3488.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in polyfile/magic.py:841-859 and compare its text_test_context behavior with file/src/ascmagic.c. Check the bundled text definitions and file/tests/ for matches involving bytes above 0x7F, then determine whether text-test offsets should remain file offsets or refer to the decoded buffer. Done means the chosen offset intent is documented and covered by a regression test.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.