You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by "Maruan Sahyoun (Jira)" <ji...@apache.org> on 2020/12/24 09:10:00 UTC
[jira] [Commented] (PDFBOX-5056) Double checked locking done wrong
in several places
[ https://issues.apache.org/jira/browse/PDFBOX-5056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17254479#comment-17254479 ]
Maruan Sahyoun commented on PDFBOX-5056:
----------------------------------------
for some of these such as {{Standard14Fonts}} and {{COSName}} we could look into utilizing Enums - maybe as part of PDFBox 3.0 - thoughts?
> Double checked locking done wrong in several places
> ---------------------------------------------------
>
> Key: PDFBOX-5056
> URL: https://issues.apache.org/jira/browse/PDFBOX-5056
> Project: PDFBox
> Issue Type: Bug
> Affects Versions: 2.0.22
> Reporter: Mike Kaplinskiy
> Priority: Major
>
> There's several places inside pdfbox where double-checked locking is done wrong. Specifically, double checked locking is the pattern of:
> {code:java}
> private volatile boolean doneInit = false;
> if (!doneInit) {
> synchronized (this) {
> if (!doneInit) {
> ... do init
> doneInit = true;
> }
> }
> }{code}
> Common issues are - lack of {{volatile}} or the volatile set not being last. Here are the cases I found so far:
> * [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/color/PDDeviceCMYK.java#L60] - {{volatile}} set isn't the last statement in the block.
> * [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/color/PDDeviceRGB.java#L48] - {{volatile}} set isn't the last statement in the block.
> * [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java#L162-L167] - {{initialized}} isn't {{volatile}}
> * [https://github.com/apache/pdfbox/blob/947966ea830ff91c61a6740ca1787eb795b5ca95/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/encoding/Encoding.java#L132-L149] - {{names}} isn't {{volatile}} and the second check is missing (which might be harmless)
> * [https://github.com/apache/pdfbox/blob/6c8526bab8b7ca399721e067d065c1f272f97644/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/Standard14Fonts.java#L172-L190] - {{fonts}} isn't even locked and it's a vanilla hashmap that can be modified by other threads.
> * [https://github.com/apache/pdfbox/blob/7984a52ad4d475886568593614ce566a88bf6bdd/pdfbox/src/main/java/org/apache/pdfbox/cos/COSName.java#L632-L637] - (tricky to see, but the constructor adds itself to the map) - the map isn't locked before the blind {{put}}, so unclear which invocation "wins" (not sure if this is an issue, logic wise).
> * [https://github.com/apache/pdfbox/blob/61d6a53eacdee6a40d352509105e1c8d51cfb5dc/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/FontMapperImpl.java#L415-L419] - {{fontProvider}} isn't volatile, though is used as a double checked lock marker. (Not sure if this an issue, concurrency wise).
> Fixing these one-by-one is possible and what I started doing around [https://github.com/apache/pdfbox/pull/90] - but would it make sense to make a class that does this properly? Guava has {{Suppliers.memoize}} which does the right thing - it's trivial to make an equivalent without the dep in pdfbox as well if necessary.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org