Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade PDFBox and PdfBoxGraphics2D version. Also fix JDK 11 build #279

Merged

Conversation

rototor
Copy link
Contributor

@rototor rototor commented Oct 7, 2018

No description provided.

@rototor
Copy link
Contributor Author

rototor commented Oct 7, 2018

Thats strange... The tests pass when running them in IDEA, but they fail when running with maven ...
I am not sure if this is related to the PDFBox upgrade.

@rototor
Copy link
Contributor Author

rototor commented Oct 7, 2018

This pull request got a little bit bigger now than thought ...

I've fixed the javadoc for JDK 11 and now mvn install also works with JDK 11.

@rototor rototor changed the title Upgrade PDFBox and PdfBoxGraphics2D version. Upgrade PDFBox and PdfBoxGraphics2D version. Also fix JDK 11 build Oct 7, 2018
@danfickle
Copy link
Owner

Thanks hugely mate! I was dreading the jdk11 stuff. I’ve got the flu, but I’ll try to merge this and do a release in the next twelve hours as this is a security release of pdfbox. The release of pdfbox also incorporates a fix for font files being left unclosed if not used so I can remove our workaround code in the pdf font resolver.

Thanks again and feel free to add any commits in the next few hours while I try to sleep.

@rototor
Copy link
Contributor Author

rototor commented Oct 7, 2018

No hurry from my side, the CVE in PDFBox is about reading special crafted PDF files, which can burn many CPU cycles and theirfor DOS a system. As this project is mostly about writing PDFs we are not directly affected

But this project should benefit from the LosslessFactory improvments I contributed to PDFBox. The image compression got better. (See https://issues.apache.org/jira/browse/PDFBOX-4184)

Get well soon!

@danfickle
Copy link
Owner

Thanks. I’m impressed with your willingness to get dirty with the details of image compression. If you want to do the same for jpeg, I notice that in JPEGFactory it appears that the image is just decoded to get colour space, width and height. Width and height can be obtained without reading the pixels through the ImageReader api which just leaves color space, for which I couldn’t find a reliable built in api.

P.s: Found some code here:
https://github.com/drewnoakes/metadata-extractor/tree/master/Source/com/drew/imaging/jpeg

@danfickle danfickle merged commit d966bdd into danfickle:open-dev-v1 Oct 8, 2018
@danfickle
Copy link
Owner

NOTE to self:
I think the only reason we need JAXB dependency is the Base64 decoding in com.openhtmltopdf.util.ImageUtil but we have an alternative implementation of Base64 decoding in com/openhtmltopdf/protocols/data/DataURLConnection.java which we could perhaps use or find a more robust source implementation.

@Jugen
Copy link

Jugen commented Oct 8, 2018

Not sure if it's more robust but java.util has a Base64 class (since JDK1.8)

@rototor
Copy link
Contributor Author

rototor commented Oct 8, 2018

@Jugen If we just upper the required java version of this project to JDK 1.8 than we can just use java.util.Base64 and drop the JAXB dependency. I'm at least on JDK 1.8 with all my project for some years now, and on JDK 10 with my main project (I can't get on JDK 11 yet, as guice/cglib is not yet there; Those low level libraries always need an ASM upgrade...).

I would be fine with a JDK 1.8 requirement. But this is on @danfickle to decide.

@danfickle I'm in the process of migrating my main project from iText to PDFBox. For that I need feature parity, at least for the features I need. And writing images with color profiles into a PDF is just something I need for my main project.

@rototor rototor deleted the upgrade_graphics2d_pdfbox_2.0.12 branch October 8, 2018 06:39
@danfickle
Copy link
Owner

Lambdas sure are tempting... I’ll start asking if anyone is still on 7.

@danfickle
Copy link
Owner

Ok, I’ve asked the java 8 question.

@zimmi zimmi mentioned this pull request Nov 30, 2018
danfickle added a commit that referenced this pull request Nov 30, 2018
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.

3 participants