From 72522a5127dd8d456833dc20d4daf6dfd86f6595 Mon Sep 17 00:00:00 2001 From: Patrick Corless Date: Mon, 31 Aug 2026 21:39:49 -0600 Subject: [PATCH 1/2] PDFBOX-6251: Keep a CMap's own cid mappings ahead of the inherited ones A CMap that begins with usecmap is meant to override the base CMap, but both the cidchar/cidrange mappings were merged into the same collections as the importing CMap's own. The range list is consulted first-match, so an inherited cidrange that covers a code beat the cidchar the importing CMap declared for it, and the override was silently lost. ETenms-B5-H and UniJIS-UCS2-HW-H are affected among the bundled predefined CMaps -- ETenms-B5-H maps 0x20-0x7e to the proportional Latin cids 1-95, but ETen-B5-H's inherited range won, so 0x41 resolved to cid 13681 (the fullwidth 'A') instead of 34. Together with their vertical variants, 32 of the 100 bundled CMaps carried lost overrides. Inherited mappings now live in their own map and range list, consulted only after the CMap's own map and ranges. Chained usecmap keeps nearest- wins ordering: the map merge is last-wins so the nearer CMap is applied after the deeper one, and the range list is first-wins so the nearer CMap's ranges are added ahead. Co-Authored-By: Claude Opus 5 --- .../java/org/apache/fontbox/cmap/CMap.java | 79 +++++++++++++------ .../apache/fontbox/cmap/TestCMapParser.java | 43 ++++++++++ 2 files changed, 98 insertions(+), 24 deletions(-) diff --git a/fontbox/src/main/java/org/apache/fontbox/cmap/CMap.java b/fontbox/src/main/java/org/apache/fontbox/cmap/CMap.java index df50a55b1ea..b0ff1eac773 100644 --- a/fontbox/src/main/java/org/apache/fontbox/cmap/CMap.java +++ b/fontbox/src/main/java/org/apache/fontbox/cmap/CMap.java @@ -65,6 +65,14 @@ public class CMap private final Map> codeToCid = new HashMap<>(); private final List codeToCidRanges = new ArrayList<>(); + // CID mappings inherited through usecmap, kept apart from this CMap's own mappings because they + // rank below them: a CMap that uses another may redefine any of the codes it inherits, and those + // redefinitions have to win. Merging the two would make the outcome depend on parse order, and + // usecmap is read from the header before the CMap's own mappings, so the inherited ones would + // always be found first. + private final Map> inheritedCodeToCid = new HashMap<>(); + private final List inheritedCodeToCidRanges = new ArrayList<>(); + // inverted map private final Map unicodeToByteCodes = new HashMap<>(); @@ -85,7 +93,8 @@ public class CMap */ public boolean hasCIDMappings() { - return !codeToCid.isEmpty() || !codeToCidRanges.isEmpty(); + return !codeToCid.isEmpty() || !codeToCidRanges.isEmpty() + || !inheritedCodeToCid.isEmpty() || !inheritedCodeToCidRanges.isEmpty(); } /** @@ -241,13 +250,19 @@ public int toCID(byte[] code) { return 0; } - Integer cid = null; - Map codeToCidMap = codeToCid.get(code.length); - if (codeToCidMap != null) + Integer cid = lookup(codeToCid, code.length, toInt(code)); + if (cid != null) + { + return cid; + } + int fromRanges = toCIDFromRanges(codeToCidRanges, code); + if (fromRanges != 0) { - cid = codeToCidMap.get(toInt(code)); + return fromRanges; } - return cid != null ? cid : toCIDFromRanges(code); + // nothing of this CMap's own matched, fall back to whatever it inherited + cid = lookup(inheritedCodeToCid, code.length, toInt(code)); + return cid != null ? cid : toCIDFromRanges(inheritedCodeToCidRanges, code); } /** @@ -290,13 +305,19 @@ public int toCID(int code, int length) { return 0; } - Integer cid = null; - Map codeToCidMap = codeToCid.get(length); - if (codeToCidMap != null) + Integer cid = lookup(codeToCid, length, code); + if (cid != null) + { + return cid; + } + int fromRanges = toCIDFromRanges(codeToCidRanges, code, length); + if (fromRanges != 0) { - cid = codeToCidMap.get(code); + return fromRanges; } - return cid != null ? cid : toCIDFromRanges(code, length); + // nothing of this CMap's own matched, fall back to whatever it inherited + cid = lookup(inheritedCodeToCid, length, code); + return cid != null ? cid : toCIDFromRanges(inheritedCodeToCidRanges, code, length); } /** @@ -306,9 +327,15 @@ public int toCID(int code, int length) * @return CID */ - private int toCIDFromRanges(int code, int length) + private static Integer lookup(Map> mappings, int length, int code) { - for (CIDRange range : codeToCidRanges) + Map codeToCidMap = mappings.get(length); + return codeToCidMap != null ? codeToCidMap.get(code) : null; + } + + private static int toCIDFromRanges(List cidRanges, int code, int length) + { + for (CIDRange range : cidRanges) { int ch = range.map(code, length); if (ch != -1) @@ -326,9 +353,9 @@ private int toCIDFromRanges(int code, int length) * @return CID */ - private int toCIDFromRanges(byte[] code) + private static int toCIDFromRanges(List cidRanges, byte[] code) { - for (CIDRange range : codeToCidRanges) + for (CIDRange range : cidRanges) { int ch = range.map(code); if (ch != -1) @@ -476,15 +503,19 @@ void useCmap(CMap cmap) } unicodeToByteCodes.put(v, bar); }); - cmap.codeToCid.forEach((key, value) -> - { - Map existingMapping = codeToCid.putIfAbsent(key, value); - if (existingMapping!=null) - { - existingMapping.putAll(value); - } - }); - codeToCidRanges.addAll(cmap.codeToCidRanges); + // CID mappings are inherited, not merged: they rank below anything this CMap defines itself + // (see inheritedCodeToCid). The copy is deliberate - sharing the other CMap's collections + // would let a later addCIDMapping here mutate a cached predefined CMap. + // A usecmap chain stays nearest-first, which needs opposite orderings for the two structures: + // the maps are consulted by key so putAll is last-wins, and the other CMap's own mappings go + // in after the ones it inherited; the range list is scanned first-match, so the other CMap's + // own ranges go in ahead of the ones it inherited. + cmap.inheritedCodeToCid.forEach((length, mappings) -> + inheritedCodeToCid.computeIfAbsent(length, k -> new HashMap<>()).putAll(mappings)); + cmap.codeToCid.forEach((length, mappings) -> + inheritedCodeToCid.computeIfAbsent(length, k -> new HashMap<>()).putAll(mappings)); + inheritedCodeToCidRanges.addAll(cmap.codeToCidRanges); + inheritedCodeToCidRanges.addAll(cmap.inheritedCodeToCidRanges); maxCodeLength = Math.max(maxCodeLength, cmap.maxCodeLength); minCodeLength = Math.min(minCodeLength, cmap.minCodeLength); maxCidLength = Math.max(maxCidLength, cmap.maxCidLength); diff --git a/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java b/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java index 9d860dcbbdc..fb3c69c5fc4 100644 --- a/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java +++ b/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java @@ -261,4 +261,47 @@ void testBadIncrement() throws IOException CMap cmap = parser.parse(new RandomAccessReadBuffer(cmapData)); assertNotNull(cmap); } + + /** + * A CMap that redefines a code it inherits through usecmap must win over the CMap it uses. + * + * ETenms-B5-H exists only to do that: it uses ETen-B5-H and then remaps 0x20-0x7E to the + * proportional latin CIDs 1-95, where the parent maps them to the full width forms at 13648+. + */ + @Test + void testUseCmapOwnMappingsWin() throws IOException + { + CMap parent = new CMapParser().parsePredefined("ETen-B5-H"); + assertEquals(13681, parent.toCID(0x41, 1), "ETen-B5-H maps 0x41 to the full width form"); + + CMap cMap = new CMapParser().parsePredefined("ETenms-B5-H"); + assertEquals(34, cMap.toCID(0x41, 1), "ETenms-B5-H overrides 0x41 to the proportional form"); + assertEquals(1, cMap.toCID(0x20, 1), "ETenms-B5-H overrides 0x20 to the proportional form"); + + // codes the CMap does not redefine still come from the one it uses + assertEquals(parent.toCID(new byte[] { (byte) 0xA1, 0x40 }), + cMap.toCID(new byte[] { (byte) 0xA1, 0x40 }), + "an inherited code is unaffected"); + + // UniJIS-UCS2-HW-H likewise overrides its parent's proportional latin with the half width forms + CMap halfWidth = new CMapParser().parsePredefined("UniJIS-UCS2-HW-H"); + assertEquals(34, new CMapParser().parsePredefined("UniJIS-UCS2-H").toCID(0x41, 2), + "UniJIS-UCS2-H maps 0x0041 to the proportional form"); + assertEquals(264, halfWidth.toCID(0x41, 2), + "UniJIS-UCS2-HW-H overrides 0x0041 to the half width form"); + } + + /** + * The override has to survive a chain of usecmap: ETenms-B5-V defines no mappings of its own, it + * uses ETenms-B5-H, which in turn uses ETen-B5-H. + */ + @Test + void testUseCmapChainKeepsNearestMapping() throws IOException + { + CMap cMap = new CMapParser().parsePredefined("ETenms-B5-V"); + + assertEquals(1, cMap.getWMode(), "ETenms-B5-V is vertical"); + assertEquals(34, cMap.toCID(0x41, 1), + "ETenms-B5-V inherits the proportional override from ETenms-B5-H, not ETen-B5-H"); + } } From f9ead3a8041423c05b0b30a744fe0f9e578626cc Mon Sep 17 00:00:00 2001 From: Patrick Corless Date: Mon, 31 Aug 2026 21:39:49 -0600 Subject: [PATCH 2/2] PDFBOX-6251: Cover the remaining usecmap cid lookup paths The first commit's two tests left the lookup order only partly guarded. Identity-V is the one predefined CMap that declares no cid mappings of its own, so every lookup it answers now goes through the inherited collections, and it is the only reason hasCIDMappings has to consider them at all. It had no test: testIdentity only covers Identity-H. ETenms-B5-V does declare mappings of its own, six cidchars and twelve cidranges for the punctuation whose vertical form differs, so the chain test's javadoc saying it declares none was wrong. Those mappings also cover the case of a CMap's own mappings beating the ranges it inherits from two levels up. The remaining two cases have no predefined CMap that exercises them, so they are built by hand: a cidrange of the CMap's own beating an inherited cidchar, and the fact that what a CMap inherits is copied rather than shared, which matters because predefined CMaps are cached and handed out repeatedly. Also assert the override through the byte[] overload of toCID, which repeats the lookup order of the int one and was only checked for a pass through. Five of the six usecmap tests now fail without the CMap change; the Identity-V one passes either way and is a regression guard for the new path. Co-Authored-By: Claude Opus 5 --- .../apache/fontbox/cmap/TestCMapParser.java | 104 +++++++++++++++++- 1 file changed, 102 insertions(+), 2 deletions(-) diff --git a/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java b/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java index fb3c69c5fc4..3ceac8bb81e 100644 --- a/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java +++ b/fontbox/src/test/java/org/apache/fontbox/cmap/TestCMapParser.java @@ -283,6 +283,11 @@ void testUseCmapOwnMappingsWin() throws IOException cMap.toCID(new byte[] { (byte) 0xA1, 0x40 }), "an inherited code is unaffected"); + // the byte[] overload repeats the lookup order of the int one, so check the override there too + assertEquals(13681, parent.toCID(new byte[] { 0x41 })); + assertEquals(34, cMap.toCID(new byte[] { 0x41 }), + "the byte[] overload has to prefer the CMap's own mapping as well"); + // UniJIS-UCS2-HW-H likewise overrides its parent's proportional latin with the half width forms CMap halfWidth = new CMapParser().parsePredefined("UniJIS-UCS2-HW-H"); assertEquals(34, new CMapParser().parsePredefined("UniJIS-UCS2-H").toCID(0x41, 2), @@ -292,8 +297,8 @@ void testUseCmapOwnMappingsWin() throws IOException } /** - * The override has to survive a chain of usecmap: ETenms-B5-V defines no mappings of its own, it - * uses ETenms-B5-H, which in turn uses ETen-B5-H. + * The override has to survive a chain of usecmap: ETenms-B5-V uses ETenms-B5-H, which in turn + * uses ETen-B5-H. A code that only the middle CMap redefines has to keep that redefinition. */ @Test void testUseCmapChainKeepsNearestMapping() throws IOException @@ -304,4 +309,99 @@ void testUseCmapChainKeepsNearestMapping() throws IOException assertEquals(34, cMap.toCID(0x41, 1), "ETenms-B5-V inherits the proportional override from ETenms-B5-H, not ETen-B5-H"); } + + /** + * Both kinds of mapping a CMap declares have to beat the ranges it inherits. ETenms-B5-V + * declares six cidchars and twelve cidranges for the punctuation whose vertical form differs, + * on top of the horizontal forms it inherits from ETenms-B5-H and ETen-B5-H. + * + * The cidchars were already resolved correctly before the inherited mappings were separated out, + * a cidchar being consulted ahead of any range either way, so they are here as a guard rather + * than as a second reproducer. + */ + @Test + void testUseCmapOwnMappingsBeatInheritedRanges() throws IOException + { + CMap horizontal = new CMapParser().parsePredefined("ETenms-B5-H"); + CMap vertical = new CMapParser().parsePredefined("ETenms-B5-V"); + + // the horizontal forms come from an inherited range in both CMaps + assertEquals(110, horizontal.toCID(0xA14B, 2)); + assertEquals(111, horizontal.toCID(0xA14C, 2)); + assertEquals(121, horizontal.toCID(0xA156, 2)); + + // ETenms-B5-V's own cidchars replace them with the vertical forms + assertEquals(13646, vertical.toCID(0xA14B, 2), "own cidchar has to beat the inherited range"); + assertEquals(109, vertical.toCID(0xA14C, 2), "own cidchar has to beat the inherited range"); + assertEquals(312, vertical.toCID(0xA156, 2), "own cidchar has to beat the inherited range"); + + // and its own cidranges likewise, two usecmap levels down + assertEquals(128, horizontal.toCID(0xA15D, 2)); + assertEquals(130, vertical.toCID(0xA15D, 2), "own cidrange has to beat the inherited range"); + } + + /** + * Identity-V is the one predefined CMap that declares no cid mappings at all, it only uses + * Identity-H. Every lookup it answers is therefore an inherited one, which also makes it the + * case that proves hasCIDMappings has to account for what a CMap inherited. + */ + @Test + void testUseCmapOnlyInheritedMappings() throws IOException + { + CMap cMap = new CMapParser().parsePredefined("Identity-V"); + + assertEquals(1, cMap.getWMode(), "Identity-V is vertical"); + assertTrue(cMap.hasCIDMappings(), "Identity-V has cid mappings, all of them inherited"); + + assertEquals(65, cMap.toCID(new byte[] { 0, 65 }), "Identity-V CID 65"); + assertEquals(12345, cMap.toCID(new byte[] { 0x30, 0x39 }), "Identity-V CID 12345"); + assertEquals(0xFFFF, cMap.toCID(new byte[] { (byte) 0xFF, (byte) 0xFF }), + "Identity-V CID 0xFFFF"); + assertEquals(12345, cMap.toCID(0x3039, 2), "Identity-V CID 12345"); + } + + /** + * Predefined CMaps are cached and handed out repeatedly, so what a CMap inherits has to be a + * copy: adding a mapping to the importing CMap must not reach back into the one it used. + */ + @Test + void testUseCmapDoesNotShareMappingsWithTheUsedCMap() throws IOException + { + CMap used = new CMap(); + used.addCIDMapping(new byte[] { 0x41 }, 100); + used.addCIDRange(new byte[] { 0x50 }, new byte[] { 0x5F }, 200); + + CMap cMap = new CMap(); + cMap.useCmap(used); + assertEquals(100, cMap.toCID(0x41, 1), "the mapping is inherited"); + assertEquals(205, cMap.toCID(0x55, 1), "the range is inherited"); + + cMap.addCIDMapping(new byte[] { 0x41 }, 300); + cMap.addCIDRange(new byte[] { 0x50 }, new byte[] { 0x5F }, 400); + + assertEquals(300, cMap.toCID(0x41, 1), "the CMap's own mapping wins"); + assertEquals(405, cMap.toCID(0x55, 1), "the CMap's own range wins"); + assertEquals(100, used.toCID(0x41, 1), "the used CMap must not have been modified"); + assertEquals(205, used.toCID(0x55, 1), "the used CMap must not have been modified"); + } + + /** + * Everything a CMap declares outranks everything it inherits, so a cidrange of its own has to + * beat an inherited cidchar too, not just an inherited cidrange. No predefined CMap pairs the + * two that way round, hence the hand built pair here. + */ + @Test + void testUseCmapOwnRangeBeatsInheritedChar() throws IOException + { + CMap used = new CMap(); + used.addCIDMapping(new byte[] { 0x41 }, 100); + + CMap cMap = new CMap(); + cMap.useCmap(used); + cMap.addCIDRange(new byte[] { 0x40 }, new byte[] { 0x4F }, 200); + + assertEquals(201, cMap.toCID(0x41, 1), "the CMap's own range has to beat the inherited char"); + assertEquals(200, cMap.toCID(0x40, 1), "a code the used CMap says nothing about"); + assertEquals(100, used.toCID(0x41, 1), "the used CMap must not have been modified"); + } }