Skip to content

fix(outlook msg): strip NUL terminators from UTF-16 string properties - #2541

Open
fei (feiiiiii5) wants to merge 1 commit into
microsoft:mainfrom
feiiiiii5:fix/msg-unicode-nul-terminator
Open

fei (feiiiiii5) wants to merge 1 commit into
microsoft:mainfrom
feiiiiii5:fix/msg-unicode-nul-terminator

Conversation

@feiiiiii5

Copy link
Copy Markdown

Problem

A .msg property stored as PT_UNICODE (__substg1.0_*001F) can be terminated with a NUL code unit. _get_stream_data read the stream and then did data.decode("utf-16-le").strip() — and str.strip() does not remove U+0000. The terminator therefore survived into the converted Markdown:

terminator=b'\x00\x00'  ->  title repr: 'Confirmación de la reunión\x00'

Worse when the terminator is a single NUL byte: the buffer becomes odd-length, utf-16-le raises, and the existing fallback re-decodes the shifted bytes as UTF-8 with errors ignored, so non-ASCII text is silently destroyed — the printed repro loses the ó in Confirmación.

This premise is already accepted in the same file: the 8-bit helper at _outlook_msg_converter.py:281-283 documents that "some writers include trailing NUL terminators" and strips them, and merged #2295 applied that fix to the 001E path only. The 001F path was left behind.

Change

Trim the padding in _get_stream_data before decoding, one UTF-16 code unit at a time: drop a single odd trailing byte first, then b"\x00\x00" pairs. Removing pairs (rather than a whole run of NUL bytes) is what keeps the last real character intact — a lone 0x00 byte belongs to the preceding character and must not be eaten on its own.

Testing

New packages/markitdown/tests/test_outlook_msg_unicode_terminators.py, reusing the _FakeOleFileIO harness style already used by the msg tests: empty-string terminator variants for a populated property and for an empty property.

Base control — src/markitdown/converters/_outlook_msg_converter.py restored from 945314a45ddbe02935f2fd287b797dc0ba4a01e4 with git diff on that file printing nothing, so the run measures upstream code with the new tests kept:

$ python -m pytest tests/test_outlook_msg_unicode_terminators.py -q
5 failed, 1 passed in 0.18s
FAILED ...::test_unicode_terminators_are_removed[\x00]
FAILED ...::test_unicode_terminators_are_removed[\x00\x00]
FAILED ...::test_unicode_terminators_are_removed[\x00\x00\x00]
FAILED ...::test_empty_unicode_properties_are_omitted[\x00]
FAILED ...::test_empty_unicode_properties_are_omitted[\x00\x00]

With the patch applied, from packages/markitdown with PYTHONPATH pointing at this tree's src:

$ python -m pytest tests/test_outlook_msg_unicode_terminators.py -q
6 passed in 0.30s

No regression in the msg-related tests, same selection at both revisions:

base:  5 failed, 44 passed, 826 deselected, 8 errors   (pytest tests -k "msg or outlook")
head:  0 failed, 49 passed, 826 deselected, 8 errors

The 8 collection errors are the same files at base and head (test_pptx_svg.py, test_xlsx_images.py, ...) and come from optional extras missing in my environment, not from this diff — nothing in tests/ that imports the msg converter is among them.

Formatting: black --check with the repo's pinned 23.7.0 from .pre-commit-config.yaml2 files would be left unchanged.

Blast radius: _get_stream_data has exactly one call site (:252, the %s001F string property), so no attachment or other binary stream passes through the new trim.

A PT_UNICODE property terminated with NUL kept its terminator because str.strip() does not remove U+0000, and an odd-length buffer failed utf-16-le and fell through to the UTF-8 branch, which destroyed non-ASCII characters. The ANSI sibling at :281 already documents this premise and microsoft#2295 fixed only the 001E path; this applies the same trim to the 001F stream, one code unit at a time so the final character is never eaten.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant