Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4392 +/- ##
=======================================
Coverage 98.87% 98.87%
=======================================
Files 59 59
Lines 12299 12311 +12
Branches 2342 2344 +2
=======================================
+ Hits 12161 12173 +12
Misses 68 68
Partials 70 70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
OK so now this should be finished. I noticed a lot of small opportunities for improvement: one small security fix, a lot of early returns and less indentation, less passing around of variables and simpler method signatures, with a net addition of 7 lines of code to _cmap.py, three of which are whitespace to make things more readable, and another four because I factored out the new _correct_encoding() method :-) Anyway, I think this should be good to go! |
…ding For a Type1 font that embeds a CFF program, ISO 32000-2:2020, 9.6.6.1 makes the program's built-in encoding only the default base encoding: a named /Encoding or a /BaseEncoding replaces it, and /Differences override the codes they list. The map derived with fontTools was handed to get_encoding() as if it came from a /ToUnicode CMap. Every code the program defines became chr(code) and was then mapped through the program's glyph names, so the font dictionary's own /Encoding was ignored and documents relying on /BaseEncoding + /Differences were extracted as garbage. This tests for py-pdf#4343. Was: Signed-off-by: Ak4shh <akashposa40@gmail.com> Is: Cherry-picked from: Ak4shh <akashposa40@gmail.com>
|
Ugh... While I was trying to find out what one of the original test was covering, I discovered that the code for deriving an encoding from a Type1 font doesn't even always work. If possible, I'll add a small bugfix for that one as well... Here's the bug:
(P. 27, Chapter 3, of the PostSript Language Reference third edition.) So, when the character encoding lines are as follows:
then the slash should be seen as a starting a new token. However, the current code only splits on space here: That line needs to be: iss2290.pdf accidentally is the only file I tested, and the only file that has this encoding syntax. |
…ng() This patch refactors the code that parses embedded font encodings. The reason that this code exists is that, for simple fonts, the encoding of an embedded font resource itself can be used as an encoding in the PDF. Previously, this code was called in the _parse_to_unicode() method, when an actual "/ToUnicode" CMap was missing from the font resource. This is incorrect in various ways. First, conceptually, an encoding is not the same as a ToUnicode CMap. The _encoding_ describes the relation between a character code and an actual glyph, while the a ToUnicode CMap describes the relation between PDF byte code and the unicode code points it represents. Second, because the embedded font was only parsed _after_ the encoding dictionary of a font dictionary was parsed, and applied afterwards to both encoding and character_map / map_dict, it overwrote a differences encoding, where really it should be the other way around - the differences should outrank the embedded font encoding. This is described in py-pdf#4343 This patch refactors the code for deriving an encoding from an embedded font. It moves that code to a separate method, which then is only called when a font dictionary with an embedded font file: - Does not define a usable "/Encoding" field; or - Defines an "/Encoding" field with "/Differences" but without "/BaseEncoding". This is in line with Paragraph 9.6.6 of the PDF Specification version 1.7, which deals with simple font encoding. To quote some of the pertinent sentences: "[E]every font program shall have a built-in encoding. Under certain circumstances, a PDF font dictionary may change the encoding used with the font program to match the requirements of the conforming writer generating the text being shown" (p. 262). And: "A font program’s built-in encoding may be overridden by including an Encoding entry in the PDF font dictionary. The possible encoding modifications depend on the font type. The value of the Encoding entry shall be either a named encoding (the name of one of the predefined encodings MacRomanEncoding, MacExpertEncoding, or WinAnsiEncoding) or an encoding dictionary" (p. 263). The specification names StandardEncoding, MacRomanEncoding, and WinAnsiEncoding as encoding names that a font dictionary can state and that have precedence over the encoding in the embedded font file. For the case of an encoding _dictionary_, the specification notes that such may contain a "/BaseEncoding" field, which can name one of the named encodings, and a "/Differences" field that denotes all character codes that are different in the font dictionary in question. In the absence of the "/BaseEncoding" field, the specification says: "[...] the Differences entry shall describe differences from an implicit base encoding. For a font program that is embedded in the PDF file, the implicit base encoding shall be the font program’s built-in encoding. [...] Otherwise, for a nonsymbolic font, it shall be StandardEncoding, and for a symbolic font, it shall be the font’s built-in encoding" (Table 114, p. 263).
When we derive an encoding from an embedded font file, this always happens only for simple fonts, which means that character codes should not exceed 255. Enforce this limit and remove associated checks that were never false elsewhere.
…_character_map_from_font_file The names of these methods are nearly the same, and the added lines to define them greatly exceed the number of lines in the merged method.
…t -> encoding Now that we have moved parsing the encoding from an embedded font file to _parse_encoding(), reflect this also in the names of all associated variables, comments and doc-strings. Also, don't speak specifically about CFF programs. Later on, we might also use fonttools to parse truetype fonts, and there's no need to explicitly mention CFF font programs here anyway.
…ted_dup_line This test was designed to test that a "dup" entry missing the glyph name must be skipped, not crash. Change it so that it also tests for issue 2290, and that it doesn't crash when raising a ValueError, and that it verifies the "put" element.
Mypy complained that we redefined x in parse_encoding. This variable occurred twice in the exact same procedure that overwrote encoding values with values from the embedded font encoding where applicable. Move this code to a separate method to make prevent redefining x.
Simplify the method signature for _parse_encoding_from_embedded_font, and add early returns to improve readability. We were passing around embedded_encoding and int_entry at various places from _parse_encoding. Don't do this. Also more strictly type embedded_encoding as dict[str, str] (was dict[Any, Any]). Also, _parse_encoding_from_embedded_font tries to read a font file to derive its encoding. For a CFF font, it is possible that this is a CID (Type0) font or a Type 1 font, as indicated by the Subtype field in the FontFile3 stream, hence the reason that we checked for the value of Type1C in this field. However, we only get to this check after we have checked that the font resource itself is Type1, meaning that we can never encounter a CID-type CFF font here anyway. Drop the check tor Type1C. Also make sure that "/FontDescriptor" is an instance of DictionaryObject.
Bug 2290 occurred as follows:
if words[3] != b"put":
IndexError: list index out of range
This was then fixed by checking len(words) > 3. However, this
did only work around the actual bug and it led to a failure to
devode the embedded font file.
Here's the actual bug. According to p. 27 of the PostSript
Language Reference third edition (Chapter 3):
"The characters (, ), <, >, [, ], {, }, /, and % are special.
They delimit syntactic entities such as strings, procedure bodies,
name literals, and comments. Any of these characters terminates the
entity preceding it and is not included in the entity."
So, when the character encoding lines are as follows, as is the
case in this issue:
'dup 61/equal put',
'dup 62/greater put',
'dup 63/question put',
'dup 64/at put',
'dup 65/A put',
'dup 66/B put',
'dup 67/C put',
'dup 68/D put',
'dup 69/E put',
'dup 70/F put',
'dup 71/G put',
'dup 72/H put',
then the slash should be seen as a starting a new entity. However,
we only split on space here:
txt = encoding_part[1] # To get the encoding part
lines = txt.replace(b"\r", b"\n").split(b"\n")
for li in lines:
if li.startswith(b"dup"):
words = [_w for _w in li.split(b" ") if _w != b""] <-----
if len(words) < 3 or (len(words) > 3 and words[3] != b"put"):
continue
This patch makes that:
words = li.replace(b"/", b" /").split()
The original fix for py-pdf#2290 was here:
if len(words) > 3 and words[3] != b"put":
continue
This replaced the first line originally added to pypdf, in py-pdf#2016:
if words[3] != b"put":
continue
Then it was later elaborated for robustness reasons here py-pdf#3835:
if len(words) < 3 or (len(words) > 3 and words[3] != b"put"):
continue
Replace most of this by expanding the try, except loop to also
check for IndexError.

With recent changes, the test for issue py-pdf#2290 does not actually run across the method that derives a font from the embedded Type1 font anymore, nor does it actually check the result. Also, that method is no longer covered since these changes. Rename the test, make sure it covers the associated method as intended, make it more specific by checking that the encoding we derived actually is correct, and make the docstring more specific.
The code that parses embedded font files for encodings was slightly overzealous, which led to corrupted text extraction, see #4343 as reported by @KRF-Technologies . This PR builds on a test case written by @Ak4shh to refactor the code for parsing font files from _parse_tounicode() to _parse_encoding(), where, according to the specification, it better belongs. Thanks to @Ak4shh for the analysis!
First, this PR refactors the code that parses embedded font encodings. The reason that this code exists is that, for simple fonts, the encoding of an embedded font resource itself can be used as an encoding in the PDF. Previously, this code was called in the _parse_to_unicode() method, when an actual "/ToUnicode" CMap was missing from the font resource. This is incorrect in two ways.
First, conceptually, an encoding is not the same as a ToUnicode CMap. The encoding describes the relation between a character code and an actual glyph, while the a ToUnicode CMap describes the relation between PDF byte code and the unicode code points it represents.
Second, because the embedded font was only parsed after the encoding dictionary of a font dictionary was parsed, and applied afterwards to both encoding and character_map / map_dict, it overwrote a differences encoding, where really it should be the other way around - the differences should outrank the embedded font encoding. This is described in #4343.
As a consequence of the refactoring, the code that specifically derived an encoding from an embedded Type1 font was covered by only one test, which tested issue #2290. It appeared as if that issue was actually not covered by the file in that test, so I decided to change the wording of the test, and keep it only for coverage. In so doing, I found out that the previous fix for #2290 actually was a work-around. This PR fixes it by actually getting an encoding from the associated file's embedded font.
Finally, I refectored the code that decided the font handler for the embedded font, after I noticed that we unneccessarily passed around the variables map_dict (now: embedded_font_encoding) and int_entry. As I got going, I tightened the typing for that variable from dict[Any, Any] to dict[str, str] and added various early returns where I could. This simplified the method signatures for three methods.
Fixes:
#4343 and #2290
I consulted AI while writing this PR, which especially helped identify the underlying cause for #2290, but wrote most of the code myself.