[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.
deepakhalale · Oct 15, 2016 · ace4481 · ace4481
1 parent f7c1359
commit ace4481
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 46 deletions.
diff --git a/...ovider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java b/...ovider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java
@@ -19,9 +19,6 @@
  * 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 static DefaultRepositorySystemSession newSession()
 
         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 );

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
@@ -33,6 +33,7 @@
 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 MavenExecutionRequest setSystemProperties( Properties properties )
     {
         if ( properties != null )
         {
-            this.systemProperties = new Properties();
-            this.systemProperties.putAll( properties );
+            this.systemProperties = SystemProperties.copyProperties( properties );
         }
         else
         {

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
@@ -28,6 +28,7 @@
 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 ProjectBuildingRequest setSystemProperties( Properties systemProperties )
     {
         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
         {

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
@@ -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;
     }
+
 }
diff --git a/...el-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java b/...el-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
@@ -286,7 +286,10 @@ public DefaultModelBuildingRequest setSystemProperties( Properties systemPropert
         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
         {

diff --git a/...lder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java b/...lder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java
@@ -116,15 +116,9 @@ public DefaultSettingsBuildingRequest setSystemProperties( Properties systemProp
         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