Repository: maven Updated Branches: refs/heads/master f7c1359cf -> ace448158
[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/ace44815 Tree: http://git-wip-us.apache.org/repos/asf/maven/tree/ace44815 Diff: http://git-wip-us.apache.org/repos/asf/maven/diff/ace44815 Branch: refs/heads/master Commit: ace448158131285e5ef8fb54b96dfb3d8d05f37e Parents: f7c1359 Author: Guillaume Boué <gb...@apache.org> Authored: Sun Oct 16 01:40:46 2016 +0200 Committer: Guillaume Boué <gb...@apache.org> Committed: Sun Oct 16 01:40:46 2016 +0200 ---------------------------------------------------------------------- .../internal/MavenRepositorySystemUtils.java | 21 +++--------- .../execution/DefaultMavenExecutionRequest.java | 4 +-- .../project/DefaultProjectBuildingRequest.java | 7 ++-- .../properties/internal/SystemProperties.java | 35 ++++++++++++-------- .../building/DefaultModelBuildingRequest.java | 5 ++- .../DefaultSettingsBuildingRequest.java | 12 ++----- 6 files changed, 38 insertions(+), 46 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/maven/blob/ace44815/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 877c277..645fd1c 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 @@ -19,9 +19,6 @@ package org.apache.maven.repository.internal; * under the License. */ -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.IOException; import java.util.Properties; import org.eclipse.aether.DefaultRepositorySystemSession; @@ -130,22 +127,14 @@ public final class MavenRepositorySystemUtils session.setArtifactDescriptorPolicy( new SimpleArtifactDescriptorPolicy( true, true ) ); + final Properties systemProperties = new Properties(); + // MNG-5670 guard against ConcurrentModificationException // MNG-6053 guard against key without value - final Properties systemProperties = new Properties(); - // This relies on the fact that load/store are synchronized internally. - try ( final ByteArrayOutputStream out = new ByteArrayOutputStream() ) - { - System.getProperties().store( out, null ); - - try ( final ByteArrayInputStream in = new ByteArrayInputStream( out.toByteArray() ) ) - { - systemProperties.load( in ); - } - } - catch ( final IOException e ) + Properties sysProp = System.getProperties(); + synchronized ( sysProp ) { - throw new AssertionError( "Unexpected IO error copying system properties.", e ); + systemProperties.putAll( sysProp ); } session.setSystemProperties( systemProperties ); http://git-wip-us.apache.org/repos/asf/maven/blob/ace44815/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/ace44815/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 97eb276..dafbefd 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 @@ -166,11 +167,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/ace44815/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 0a77376..f5630c1 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,24 +33,33 @@ public class SystemProperties */ public static void addSystemProperties( Properties props ) { - for ( String key : System.getProperties().stringPropertyNames() ) - { - String value = System.getProperty( key ); - // could be null if another thread concurrently removed this key (MNG-6105) - if ( value != null ) - { - props.put( key, value ); - } - } + 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/ace44815/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/ace44815/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 d917a9c..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,15 +116,9 @@ public class DefaultSettingsBuildingRequest if ( systemProperties != null ) { this.systemProperties = new Properties(); - // MNG-5670 guard against ConcurrentModificationException - for ( String key : System.getProperties().stringPropertyNames() ) - { - String value = System.getProperty( key ); - // could be null if another thread concurrently removed this key (MNG-6105) - if ( value != null ) - { - this.systemProperties.put( key, value ); - } + synchronized ( systemProperties ) + { // avoid concurrentmodification if someone else sets/removes an unrelated system property + this.systemProperties.putAll( systemProperties ); } } else
