Skip to content

Commit

Permalink
Fix concurrency issues in XStreamMarshaller
Browse files Browse the repository at this point in the history
Prior to this commit, if an instance of XStreamMarshaller was used
concurrently from multiple threads without having first invoked the
afterPropertiesSet() method, the private `xstream` field could be
initialized multiple times resulting in a ConcurrentModificationException
in XStream's internal DefaultConverterLookup.

This commit fixes these concurrency issues by making the `xstream` field
volatile and by implementing a double-checked locking algorithm in
getXStream() to avoid concurrent instance creation.

Closes gh-25017
  • Loading branch information
sbrannen committed May 6, 2020
1 parent 740f0d7 commit ce11fdf
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 13 deletions.
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -113,6 +113,7 @@
* @author Peter Meijer
* @author Arjen Poutsma
* @author Juergen Hoeller
* @author Sam Brannen
* @since 3.0
*/
public class XStreamMarshaller extends AbstractMarshaller implements BeanClassLoaderAware, InitializingBean {
Expand Down Expand Up @@ -187,7 +188,7 @@ public class XStreamMarshaller extends AbstractMarshaller implements BeanClassLo
private ClassLoader beanClassLoader = new CompositeClassLoader();

@Nullable
private XStream xstream;
private volatile XStream xstream;


/**
Expand Down Expand Up @@ -616,12 +617,21 @@ protected void customizeXStream(XStream xstream) {
* <p><b>NOTE: This method has been marked as final as of Spring 4.0.</b>
* It can be used to access the fully configured XStream for marshalling
* but not configuration purposes anymore.
* <p>As of Spring Framework 5.2.7, creation of the {@link XStream} instance
* returned by this method is thread safe.
*/
public final XStream getXStream() {
if (this.xstream == null) {
this.xstream = buildXStream();
XStream xs = this.xstream;

This comment has been minimized.

Copy link
@quaff

quaff May 7, 2020

Contributor

Can we use SingletonSupplier to eliminate boilerplate code?

This comment has been minimized.

Copy link
@sbrannen

sbrannen May 7, 2020

Author Member

Good catch, @quaff!

I honestly didn't think about using the SingletonSupplier there since we don't use it that frequently, but it's a good fit for this use case.

I'll update the code accordingly.

if (xs == null) {
synchronized (this) {
xs = this.xstream;
if (xs == null) {
xs = buildXStream();
this.xstream = xs;
}
}
}
return this.xstream;
return xs;
}


Expand Down
Expand Up @@ -21,10 +21,10 @@
import java.io.StringReader;
import java.io.StringWriter;
import java.io.Writer;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.IntStream;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
Expand Down Expand Up @@ -185,13 +185,23 @@ void marshalStaxResultXMLEventWriter() throws Exception {
@Test
void converters() throws Exception {
marshaller.setConverters(new EncodedByteArrayConverter());
byte[] buf = new byte[]{0x1, 0x2};
Writer writer = new StringWriter();
marshaller.marshal(buf, new StreamResult(writer));
assertThat(XmlContent.from(writer)).isSimilarTo("<byte-array>AQI=</byte-array>");
Reader reader = new StringReader(writer.toString());
byte[] bufResult = (byte[]) marshaller.unmarshal(new StreamSource(reader));
assertThat(Arrays.equals(buf, bufResult)).as("Invalid result").isTrue();
byte[] buf = {0x1, 0x2};

// Execute multiple times concurrently to ensure there are no concurrency issues.
// See https://github.com/spring-projects/spring-framework/issues/25017
IntStream.rangeClosed(1, 100).parallel().forEach(n -> {
try {
Writer writer = new StringWriter();
marshaller.marshal(buf, new StreamResult(writer));
assertThat(XmlContent.from(writer)).isSimilarTo("<byte-array>AQI=</byte-array>");
Reader reader = new StringReader(writer.toString());
byte[] bufResult = (byte[]) marshaller.unmarshal(new StreamSource(reader));
assertThat(bufResult).as("Invalid result").isEqualTo(buf);
}
catch (Exception ex) {
throw new RuntimeException(ex);
}
});
}

@Test
Expand Down

0 comments on commit ce11fdf

Please sign in to comment.