You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by GitBox <gi...@apache.org> on 2021/05/12 19:37:06 UTC
[GitHub] [pdfbox] larrylynn-wf opened a new pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
larrylynn-wf opened a new pull request #117:
URL: https://github.com/apache/pdfbox/pull/117
### PROBLEM
New versions of pdfbox are too restrictive and refuse to embed some fonts with OS2 tables version 0-2 that have multiple permission but set.
### SOLUTION
tweak the isEmbeddingPermitted() method to return the proper boolean when multiple permission bits are set (like 0110 = 6). Add unit tests to exercise all legal permission combinations
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] larrylynn-wf commented on a change in pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
Posted by GitBox <gi...@apache.org>.
larrylynn-wf commented on a change in pull request #117:
URL: https://github.com/apache/pdfbox/pull/117#discussion_r631346775
##########
File path: pdfbox/pom.xml
##########
@@ -75,6 +75,12 @@
<artifactId>jbig2-imageio</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
Review comment:
I didn't see any existing tests using a mocking library. I'm not sure if this is intentional. I'm also not sure if pdfbox maintainers have a policy against adding new dependencies. If mockito is undesirable for some reason, I'll have to come up with a different way to test my proposed code change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] asfgit closed pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #117:
URL: https://github.com/apache/pdfbox/pull/117
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] larrylynn-wf commented on a change in pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
Posted by GitBox <gi...@apache.org>.
larrylynn-wf commented on a change in pull request #117:
URL: https://github.com/apache/pdfbox/pull/117#discussion_r631379275
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/TrueTypeEmbedder.java
##########
@@ -136,13 +136,20 @@ public final void buildFontFile2(InputStream ttfStream) throws IOException
/**
* Returns true if the fsType in the OS/2 table permits embedding.
*/
- private boolean isEmbeddingPermitted(TrueTypeFont ttf) throws IOException
+ protected boolean isEmbeddingPermitted(TrueTypeFont ttf) throws IOException
{
if (ttf.getOS2Windows() != null)
{
int fsType = ttf.getOS2Windows().getFsType();
- if ((fsType & OS2WindowsMetricsTable.FSTYPE_RESTRICTED) ==
- OS2WindowsMetricsTable.FSTYPE_RESTRICTED)
+ int maskedFsType = fsType & 0x000F;
+ // use two's complement arithmetic to check if fsType is a power of 2
+ // all legal fsType combinations for fonts having a version 3 OS2 table are powers of 2
+ // there are more legal combinations for versions 0-2, but the most permissive pit takes precedence
+ boolean powerOfTwo = maskedFsType != 0 && (maskedFsType & (maskedFsType - 1)) == 0;
Review comment:
I think that it's worth pointing out that this code will fail open on an illegal configuration.
If a font with a version 3 OS2 table has multiple permission bits set, this is an illegal configuration because version 3 explicitly specifies that permission bits should be mutually exclusive. Suppose we are processing a font with version 3 and permission set to 0110. This code would allow embedding of the font with the illegal permission configuration. Essentially, it will fail open.
I think this is OK because in this case, the creator of the font has not made their wishes unambiguous and clear. What if the font creator wanted us to respect the other permission bit and not FSTYPE_RESTRICTED. I think failing open is OK, and it essentially puts the onus on font creators. If they want to lock their fonts down and prevent embedding, it is up to them to provide a legal fsType configuration that makes it clear that that is their intention.
I can see other points of view on this issue though, so I thought it was at least worth pointing out for possible discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] larrylynn-wf commented on a change in pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
Posted by GitBox <gi...@apache.org>.
larrylynn-wf commented on a change in pull request #117:
URL: https://github.com/apache/pdfbox/pull/117#discussion_r631348656
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/TrueTypeEmbedder.java
##########
@@ -136,13 +136,20 @@ public final void buildFontFile2(InputStream ttfStream) throws IOException
/**
* Returns true if the fsType in the OS/2 table permits embedding.
*/
- private boolean isEmbeddingPermitted(TrueTypeFont ttf) throws IOException
+ protected boolean isEmbeddingPermitted(TrueTypeFont ttf) throws IOException
{
if (ttf.getOS2Windows() != null)
{
int fsType = ttf.getOS2Windows().getFsType();
- if ((fsType & OS2WindowsMetricsTable.FSTYPE_RESTRICTED) ==
- OS2WindowsMetricsTable.FSTYPE_RESTRICTED)
+ int maskedFsType = fsType & 0x000F;
Review comment:
For this PR, I branched off of the trunk. I believe this is essentially the version 3.0.x pre-release codebase. However, this method is identical in trunk and the current stable release version, 2.0.23. So, I think this patch will work for both the 2.0.x codebase and the 3.0.x codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] larrylynn-wf commented on a change in pull request #117: PDFBOX-5191 | isEmbeddingPermitted() is too restrictive on TTFs with OS2 table version
Posted by GitBox <gi...@apache.org>.
larrylynn-wf commented on a change in pull request #117:
URL: https://github.com/apache/pdfbox/pull/117#discussion_r631349953
##########
File path: pdfbox/src/test/java/org/apache/pdfbox/pdmodel/font/TestFontEmbedding.java
##########
@@ -371,4 +376,116 @@ void testReuseEmbeddedSubsettedFont() throws IOException
assertEquals(text1 + " " + text2, extractedText.trim());
}
}
+
+ private class TrueTypeEmbedderTester extends TrueTypeEmbedder {
+ /**
+ * Common functionality for testing the TrueTypeFontEmbedder
+ *
+ */
+ TrueTypeEmbedderTester(PDDocument document, COSDictionary dict, TrueTypeFont ttf, boolean embedSubset) throws IOException {
+ super(document, dict, ttf, embedSubset);
+ }
+
+ @Override
+ protected void buildSubset(InputStream ttfSubset, String tag, Map<Integer, Integer> gidToCid) throws IOException {
+ // no-op. Need to define method to extend abstract class, but
+ // this method is not currently needed for testing
+ }
+ }
+
+ /**
+ * Test that we validate embedding permissions properly for all legal permissions combinations
+ *
+ * @throws IOException
+ */
+ @Test
+ void testIsEmbeddingPermittedMultipleVersons() throws IOException
Review comment:
This test is compatible with the version 3.0.x codebase. However, it looks like the testing frameworks have changed significantly between version 2.0.x and 3.0.x. This test won't work as-is in the 2.0.x codebase.
I think I need to have a discussion with the pdfbox maintainers about how they want to handle test coverage for my proposed change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org