[MNG-6105] properties.internal.SystemProperties.addSystemProperties() is not really thread-safe
Refactoring the current code setting system properties to synchronize correctly on the given ones: avoids ConcurrentModificationException and NullPointerException if the properties is modified by another thread. Project: http://git-wip-us.apache.org/repos/asf/maven/repo Commit: http://git-wip-us.apache.org/repos/asf/maven/commit/5b4b8bd9 Tree: http://git-wip-us.apache.org/repos/asf/maven/tree/5b4b8bd9 Diff: http://git-wip-us.apache.org/repos/asf/maven/diff/5b4b8bd9 Branch: refs/heads/MNG-5359-IT Commit: 5b4b8bd94c87afd2a1527d6a860e9673bdaf4a22 Parents: 1e2a80e Author: Guillaume Boué <gb...@apache.org> Authored: Sun Oct 16 01:40:46 2016 +0200 Committer: Guillaume Boué <gb...@apache.org> Committed: Sat Jan 28 14:07:59 2017 +0100 ---------------------------------------------------------------------- .../internal/MavenRepositorySystemUtils.java | 14 +++++---- .../execution/DefaultMavenExecutionRequest.java | 4 +-- .../project/DefaultProjectBuildingRequest.java | 7 ++--- .../properties/internal/SystemProperties.java | 30 ++++++++++++++------ .../building/DefaultModelBuildingRequest.java | 5 +++- .../DefaultSettingsBuildingRequest.java | 7 ++--- 6 files changed, 42 insertions(+), 25 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java ---------------------------------------------------------------------- diff --git a/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java b/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java index 5b240ef..1b11cb3 100644 --- a/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java +++ b/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java @@ -126,14 +126,18 @@ public final class MavenRepositorySystemUtils session.setArtifactDescriptorPolicy( new SimpleArtifactDescriptorPolicy( true, true ) ); + final Properties systemProperties = new Properties(); + // MNG-5670 guard against ConcurrentModificationException - Properties sysProps = new Properties(); - for ( String key : System.getProperties().stringPropertyNames() ) + // MNG-6053 guard against key without value + Properties sysProp = System.getProperties(); + synchronized ( sysProp ) { - sysProps.put( key, System.getProperty( key ) ); + systemProperties.putAll( sysProp ); } - session.setSystemProperties( sysProps ); - session.setConfigProperties( sysProps ); + + session.setSystemProperties( systemProperties ); + session.setConfigProperties( systemProperties ); return session; } http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java ---------------------------------------------------------------------- diff --git a/maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java b/maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java index 71a6894..d67061f 100644 --- a/maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java +++ b/maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java @@ -33,6 +33,7 @@ import org.apache.maven.eventspy.internal.EventSpyDispatcher; import org.apache.maven.model.Profile; import org.apache.maven.project.DefaultProjectBuildingRequest; import org.apache.maven.project.ProjectBuildingRequest; +import org.apache.maven.properties.internal.SystemProperties; import org.apache.maven.settings.Mirror; import org.apache.maven.settings.Proxy; import org.apache.maven.settings.Server; @@ -535,8 +536,7 @@ public class DefaultMavenExecutionRequest { if ( properties != null ) { - this.systemProperties = new Properties(); - this.systemProperties.putAll( properties ); + this.systemProperties = SystemProperties.copyProperties( properties ); } else { http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java ---------------------------------------------------------------------- diff --git a/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java b/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java index f439240..f1b271b 100644 --- a/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java +++ b/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java @@ -28,6 +28,7 @@ import org.apache.commons.lang3.Validate; import org.apache.maven.artifact.repository.ArtifactRepository; import org.apache.maven.model.Profile; import org.apache.maven.model.building.ModelBuildingRequest; +import org.apache.maven.properties.internal.SystemProperties; import org.eclipse.aether.RepositorySystemSession; public class DefaultProjectBuildingRequest @@ -165,11 +166,7 @@ public class DefaultProjectBuildingRequest { if ( systemProperties != null ) { - this.systemProperties = new Properties(); - synchronized ( systemProperties ) - { // avoid concurrentmodification if someone else sets/removes an unrelated system property - this.systemProperties.putAll( systemProperties ); - } + this.systemProperties = SystemProperties.copyProperties( systemProperties ); } else { http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java ---------------------------------------------------------------------- diff --git a/maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java b/maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java index aa5fed9..3d6a969 100644 --- a/maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java +++ b/maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java @@ -33,19 +33,33 @@ public class SystemProperties */ public static void addSystemProperties( Properties props ) { - for ( String key : System.getProperties().stringPropertyNames() ) - { - props.put( key, System.getProperty( key ) ); - } + props.putAll( getSystemProperties() ); } /** - * Returns System.properties copy. + * Returns a copy of {@link System#getProperties()} in a thread-safe manner. + * + * @return {@link System#getProperties()} obtained in a thread-safe manner. */ public static Properties getSystemProperties() { - Properties systemProperties = new Properties(); - addSystemProperties( systemProperties ); - return systemProperties; + return copyProperties( System.getProperties() ); + } + + /** + * Copies the given {@link Properties} object into a new {@link Properties} object, in a thread-safe manner. + * @param properties Properties to copy. + * @return Copy of the given properties. + */ + public static Properties copyProperties( Properties properties ) + { + final Properties copyProperties = new Properties(); + // guard against modification/removal of keys in the given properties (MNG-5670, MNG-6053, MNG-6105) + synchronized ( properties ) + { + copyProperties.putAll( properties ); + } + return copyProperties; } + } http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java ---------------------------------------------------------------------- diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java index a3505c9..84a68f7 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java @@ -286,7 +286,10 @@ public class DefaultModelBuildingRequest if ( systemProperties != null ) { this.systemProperties = new Properties(); - this.systemProperties.putAll( systemProperties ); + synchronized ( systemProperties ) + { // avoid concurrentmodification if someone else sets/removes an unrelated system property + this.systemProperties.putAll( systemProperties ); + } } else { http://git-wip-us.apache.org/repos/asf/maven/blob/5b4b8bd9/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java ---------------------------------------------------------------------- diff --git a/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java b/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java index 5a4824e..4bb691b 100644 --- a/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java +++ b/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java @@ -116,10 +116,9 @@ public class DefaultSettingsBuildingRequest if ( systemProperties != null ) { this.systemProperties = new Properties(); - // MNG-5670 guard against ConcurrentModificationException - for ( String key : System.getProperties().stringPropertyNames() ) - { - this.systemProperties.put( key, System.getProperty( key ) ); + synchronized ( systemProperties ) + { // avoid concurrentmodification if someone else sets/removes an unrelated system property + this.systemProperties.putAll( systemProperties ); } } else
