[jira] [Commented] (PDFBOX-5056) Double checked locking done wrong in several places

[Tilman Hausherr (Jira)]

    [ 
https://issues.apache.org/jira/browse/PDFBOX-5056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17254824#comment-17254824
 ] 

Tilman Hausherr commented on PDFBOX-5056:
-----------------------------------------

Yeah.

I was asking mostly because this double-checked locking is so full of surprises 
that I was wondering if I had missed something 😂

> 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-unsubscr...@pdfbox.apache.org
For additional commands, e-mail: dev-h...@pdfbox.apache.org