fix(outlook msg): strip NUL terminators from UTF-16 string properties - #2541
Open
fei (feiiiiii5) wants to merge 1 commit into
Open
fei (feiiiiii5) wants to merge 1 commit into
fei (feiiiiii5) wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A
.msgproperty stored asPT_UNICODE(__substg1.0_*001F) can be terminated with a NUL code unit._get_stream_dataread the stream and then diddata.decode("utf-16-le").strip()— andstr.strip()does not removeU+0000. The terminator therefore survived into the converted Markdown:Worse when the terminator is a single NUL byte: the buffer becomes odd-length,
utf-16-leraises, 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óinConfirmación.This premise is already accepted in the same file: the 8-bit helper at
_outlook_msg_converter.py:281-283documents that "some writers include trailing NUL terminators" and strips them, and merged #2295 applied that fix to the001Epath only. The001Fpath was left behind.Change
Trim the padding in
_get_stream_databefore decoding, one UTF-16 code unit at a time: drop a single odd trailing byte first, thenb"\x00\x00"pairs. Removing pairs (rather than a whole run of NUL bytes) is what keeps the last real character intact — a lone0x00byte 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_FakeOleFileIOharness 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.pyrestored from945314a45ddbe02935f2fd287b797dc0ba4a01e4withgit diffon that file printing nothing, so the run measures upstream code with the new tests kept:With the patch applied, from
packages/markitdownwithPYTHONPATHpointing at this tree'ssrc:No regression in the msg-related tests, same selection at both revisions:
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 intests/that imports the msg converter is among them.Formatting:
black --checkwith the repo's pinned 23.7.0 from.pre-commit-config.yaml→2 files would be left unchanged.Blast radius:
_get_stream_datahas exactly one call site (:252, the%s001Fstring property), so no attachment or other binary stream passes through the new trim.