[
https://issues.apache.org/jira/browse/PDFBOX-5056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255715#comment-17255715
]
Mike Kaplinskiy commented on PDFBOX-5056:
-----------------------------------------
My knowledge is a little iffy here - but it seems like the unsafe publication
problem applies. Technically Java/JVM is free to reorder the constructor code
to happen AFTER the assignment to the volatile. See
https://stackoverflow.com/questions/16178020/uninitialized-object-leaked-to-another-thread-despite-no-code-explicitly-leaking
for an example. Generally it seems like using an non-primitive field as a
volatile in a double checked lock can yield issues.
I do want to bring up two solutions. The first is - don’t use double checked
locking at all. Just use a synchronized block (with a check to skip if
initialization is complete inside the block) all the time. Using double checked
locking at all is dangerous given how many ways there are to get it wrong, and
this might be a premature optimization.
For that particular piece of code though - it seems like all the
synchronization can be removed by just checking the {{inverted}} map that
already has the keys. You wouldn’t even need to construct a hashset at all this
way.
> 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: [email protected]
For additional commands, e-mail: [email protected]
