Race condition in OdfStyleProperty.get() during concurrent style initialization

[kostyakit]

Race condition in OdfStyleProperty.get() during concurrent style initialization

Environment

• odfdom-java: 0.13.0
• Java: 11.0.30
• OS: Windows
• The same unsafe implementation is still present in the current master branch.

Problem

OdfStyleProperty stores registered properties in a shared static
TreeSet<OdfStyleProperty>:

private static TreeSet<OdfStyleProperty> m_styleProperties = new TreeSet<>();

OdfStyleProperty.get(...) reads and modifies this TreeSet without
synchronization. During concurrent first use of different generated
Style*PropertiesElement classes, their static initializers can call
OdfStyleProperty.get(...) simultaneously.

This can corrupt the internal TreeMap used by TreeSet and cause class
initialization to fail.

The affected code is still present in the current source:

https://github.com/tdf/odftoolkit/blob/master/odfdom/src/main/java/org/odftoolkit/odfdom/dom/style/props/OdfStyleProperty.java

Actual result

The failure is intermittent. A representative stack trace is:

java.lang.ExceptionInInitializerError
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:398)
Caused by: java.lang.NullPointerException
    at java.util.TreeMap.fixAfterInsertion(TreeMap.java:2257)
    at java.util.TreeMap.put(TreeMap.java:580)
    at java.util.TreeSet.add(TreeSet.java:255)
    at org.odftoolkit.odfdom.dom.style.props.OdfStyleProperty.get(OdfStyleProperty.java:64)
    at org.odftoolkit.odfdom.dom.element.style.StyleParagraphPropertiesElement.<clinit>(StyleParagraphPropertiesElement.java:144)

Once a generated element class has failed to initialize, subsequent attempts to
use it may result in NoClassDefFoundError.

Expected result

Concurrent initialization and registration of style properties must not corrupt
shared state or cause generated element classes to fail initialization.

Reproduction

Add org.odftoolkit:odfdom-java:0.13.0 to the classpath and run the following
class repeatedly in fresh JVMs:

import org.odftoolkit.odfdom.dom.OdfDocumentNamespace;
import org.odftoolkit.odfdom.dom.style.props.OdfStylePropertiesSet;
import org.odftoolkit.odfdom.dom.style.props.OdfStyleProperty;
import org.odftoolkit.odfdom.pkg.OdfName;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;

public final class StylePropertyRaceProbe {

    public static void main(String[] args) throws Exception {
        int threads = 32;
        int operationsPerThread = 5_000;
        ExecutorService pool = Executors.newFixedThreadPool(threads);
        CountDownLatch ready = new CountDownLatch(threads);
        CountDownLatch start = new CountDownLatch(1);
        List<Future<?>> futures = new ArrayList<>();

        for (int thread = 0; thread < threads; thread++) {
            final int worker = thread;
            futures.add(pool.submit(() -> {
                ready.countDown();
                start.await();
                for (int i = 0; i < operationsPerThread; i++) {
                    OdfStyleProperty.get(
                            OdfStylePropertiesSet.values()[
                                    (worker + i) % OdfStylePropertiesSet.values().length],
                            OdfName.newName(
                                    OdfDocumentNamespace.TEXT,
                                    "race-property-" + worker + "-" + i));
                }
                return null;
            }));
        }

        ready.await(30, TimeUnit.SECONDS);
        start.countDown();

        int failures = 0;
        for (Future<?> future : futures) {
            try {
                future.get(30, TimeUnit.SECONDS);
            } catch (Exception e) {
                failures++;
                e.printStackTrace(System.err);
            }
        }

        pool.shutdownNow();
        System.out.println("failures=" + failures);
        if (failures != 0) {
            System.exit(2);
        }
    }
}

In a bounded local test using fresh JVMs:

• four completed runs reproduced NullPointerException inside
  TreeMap.fixAfterInsertion, with one to four failed workers per run;
• one additional run timed out;
• a production-like cold-start test that initialized different generated
  Style*PropertiesElement classes concurrently reproduced
  ExceptionInInitializerError with the same root cause.

Root cause

OdfStyleProperty.get(...) performs compound operations on the shared static
TreeSet, including iteration, contains(...), and add(...), without any
synchronization. TreeSet and its backing TreeMap are not thread-safe.

Workaround

Sequentially initializing the standard style classes before starting concurrent
document generation prevented the failure in our tests. This is only a
workaround: it does not make OdfStyleProperty thread-safe and does not cover
properties initialized later.

Suggested direction

Make property canonicalization thread-safe, for example by synchronizing access
to the shared collection or replacing it with a suitable concurrent map keyed by
the property set and property name.

Please also consider adding a cold-start concurrency regression test that
initializes multiple generated Style*PropertiesElement classes simultaneously.

[svanteschubert]

@kostyakit Thank you for your report and my apologies for my late respond, I have been to the ODF ISO meeting and was distracted since. I aim to work on this along with a performance test, that I have in mind for a long time in the next weeks, my personal dead-line is the LibreOffice Conference in Europe mid of September! :-)