Xml SBOM parsing not thread safe

[Lajcik]

Parsing xml sboms is not done in a thread-safe manner. When running parsing concurrently in multiple threads there is a chance to randomly crash with an exception like this:

Caused by: com.fasterxml.jackson.databind.JsonMappingException: For input string: "20242024202420242024" (through reference chain: org.cyclonedx.model.Bom["metadata"])
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:401) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:360) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.wrapAndThrow(BeanDeserializerBase.java:1964) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:312) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.dataformat.xml.deser.WrapperHandlingDeserializer.deserialize(WrapperHandlingDeserializer.java:122) ~[jackson-dataformat-xml-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.dataformat.xml.deser.XmlDeserializationContext.readRootValue(XmlDeserializationContext.java:104) ~[jackson-dataformat-xml-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4917) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3725) ~[jackson-databind-2.18.2.jar:2.18.2]
	at org.cyclonedx.parsers.XmlParser.parse(XmlParser.java:86) ~[cyclonedx-core-java-10.1.0.jar:na]
	... 7 common frames omitted
Caused by: java.lang.NumberFormatException: For input string: "20242024202420242024"
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67) ~[na:na]
	at java.base/java.lang.Long.parseLong(Long.java:709) ~[na:na]
	at java.base/java.lang.Long.parseLong(Long.java:832) ~[na:na]
	at java.base/java.text.DigitList.getLong(DigitList.java:196) ~[na:na]
	at java.base/java.text.DecimalFormat.parse(DecimalFormat.java:2228) ~[na:na]
	at java.base/java.text.SimpleDateFormat.subParse(SimpleDateFormat.java:1937) ~[na:na]
	at java.base/java.text.SimpleDateFormat.parse(SimpleDateFormat.java:1545) ~[na:na]
	at java.base/java.text.DateFormat.parse(DateFormat.java:397) ~[na:na]
	at org.cyclonedx.util.TimestampUtils.parseTimestamp(TimestampUtils.java:33) ~[cyclonedx-core-java-10.1.0.jar:na]
	at org.cyclonedx.util.deserializer.MetadataDeserializer.setTimestamp(MetadataDeserializer.java:136) ~[cyclonedx-core-java-10.1.0.jar:na]
	at org.cyclonedx.util.deserializer.MetadataDeserializer.deserialize(MetadataDeserializer.java:79) ~[cyclonedx-core-java-10.1.0.jar:na]
	at org.cyclonedx.util.deserializer.MetadataDeserializer.deserialize(MetadataDeserializer.java:22) ~[cyclonedx-core-java-10.1.0.jar:na]
	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129) ~[jackson-databind-2.18.2.jar:2.18.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:310) ~[jackson-databind-2.18.2.jar:2.18.2]
	... 13 common frames omitted

The culprit seems to be the org.cyclonedx.util.TimestampUtils.parseTimestamp(TimestampUtils.java:33) method which uses a static singleton instance of SimpleDateFormat which is not thread safe. SDF relies on an instance of Calendar which is the actual culprit here.

A quick fix would be to use the new DateTimeFormatter class that is thread safe. Though you'd have to convert the parsing result to java.util.Date or rewrite the whole code to use the new time classes (like LocalDateTime) instead of java.util.Date.

A even quicker fix would be to simply make an instance of SDF (or TimestampUtils) per instance of MetadataDeserializer instead of a single static instance.

Code to reproduce the error

Below is a bit of code to reproduce this (randomly), obviously you need a bunch of sbom files in the inputdir

class Scratch {
    public static void main(String[] args) {
        ExecutorService executorService = Executors.newFixedThreadPool(20);
        List<File> files = List.of(new File("inputdir").listFiles())
        List<Future<?>> futures = new ArrayList<>();
        for (File input : files) {
            futures.add(executorService.submit(new ParseTask(input)));
        }
        // wait for all tasks to finish
        futures.forEach(f -> {
            try {
                f.get();
            } catch (InterruptedException | ExecutionException e) {
                //
            }
        });
        executorService.shutdown();
    }

    private static class ParseTask implements Runnable {
        private final File input;

        public ParseTask(File input) {
            this.input = input;
        }

        public void run() {
            try {
                Parser parser = BomParserFactory.createParser(input);
                Bom bom = parser.parse(input);
            } catch (ParseException e) {
                throw new RuntimeException(e);
            }
        }
    }
}

Workaround

The quickest workaround for now is to make a of copy the MetadataDeserializer class, replace the setTimestamp() method with a thread-safe implementation and use mixins to override the serialisation (by manipulating the mapper field on XmlParser through reflection)

        XmlParser xmlParser = new XmlParser();
        try {
            Field mapperField = XmlParser.class.getDeclaredField("mapper");
            mapperField.setAccessible(true);
            ObjectMapper mapper = (ObjectMapper) mapperField.get(xmlParser);
            mapper.registerModule(new CycloneDxMetadataThreadSafetyFixModule());
        } catch (NoSuchFieldException | IllegalAccessException e) {
            throw new RuntimeException(e);
        }

public class CycloneDxMetadataThreadSafetyFixModule extends SimpleModule {
    @Override
    public void setupModule(SetupContext context) {
        context.setMixInAnnotations(Metadata.class, MetadataMixin.class);
    }

    @JsonDeserialize(using = ThreadSafeMetadataDeserializer.class)
    public abstract static class MetadataMixin {
    }
}

// the ThreadSafeMetadataDeserializer is a vanilla copy of MetadataDeserializer that calls ThreadSafeTimestampUtils instead of TimestampUtils

public final class ThreadSafeTimestampUtils {
    public static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssX");

    private ThreadSafeTimestampUtils() {
    }

    public static Date parseTimestamp(String text) {
        try {
            TemporalAccessor parsed = FORMATTER.parse(text);
            LocalDateTime localDateTime = LocalDateTime.from(parsed);
            return Date.from(localDateTime.atZone(ZoneId.systemDefault()).toInstant());
        } catch (NullPointerException e) {
            return null;
        }
    }
}

This is obviously a pretty hacky way to fix this, but it solves the problem locally :)