CycloneDX / CycloneDX/cyclonedx-core-java

Xml SBOM parsing not thread safe

Open
#626 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
120
Forks
90
Avg merge
12h 43m
Merged PRs (30d)
18

Description

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 files = List.of(new File("inputdir").listFiles())
List> 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 :)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.