[MSHARED-417] Infinite loop when loading self-referencing properties

Submitted-by: Gabriel Belingueres <belingueres@gmail.com>

This closes #17

git-svn-id: https://svn.apache.org/repos/asf/maven/shared/trunk@1791421 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/src/main/java/org/apache/maven/shared/filtering/BaseFilter.java b/src/main/java/org/apache/maven/shared/filtering/BaseFilter.java
index cb04db2..63a3966 100644
--- a/src/main/java/org/apache/maven/shared/filtering/BaseFilter.java
+++ b/src/main/java/org/apache/maven/shared/filtering/BaseFilter.java
@@ -208,7 +208,7 @@
                 try
                 {
                     File propFile = FileUtils.resolveFile( basedir, filterFile );
-                    Properties properties = PropertyUtils.loadPropertyFile( propFile, workProperties );
+                    Properties properties = PropertyUtils.loadPropertyFile( propFile, workProperties, getLogger() );
                     filterProperties.putAll( properties );
                     workProperties.putAll( properties );
                 }
diff --git a/src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java b/src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java
index d03277c..3c8c9a5 100644
--- a/src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java
+++ b/src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java
@@ -23,10 +23,13 @@
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Properties;
 
 import org.apache.maven.shared.utils.StringUtils;
 import org.apache.maven.shared.utils.io.IOUtil;
+import org.codehaus.plexus.logging.Logger;
 
 /**
  * @author <a href="mailto:kenney@neonics.com">Kenney Westerhof</a>
@@ -57,6 +60,27 @@
     public static Properties loadPropertyFile( File propFile, Properties baseProps )
         throws IOException
     {
+        return loadPropertyFile( propFile, baseProps, null );
+    }
+
+    /**
+     * Reads a property file, resolving all internal variables, using the supplied base properties.
+     * <p>
+     * The properties are resolved iteratively, so if the value of property A refers to property B, then after
+     * resolution the value of property B will contain the value of property B.
+     * </p>
+     *
+     * @param propFile The property file to load.
+     * @param baseProps Properties containing the initial values to substitute into the properties file.
+     * @param logger Logger instance
+     * @return Properties object containing the properties in the file with their values fully resolved.
+     * @throws IOException if profile does not exist, or cannot be read.
+     *
+     * @since 3.1.2
+     */
+    public static Properties loadPropertyFile( File propFile, Properties baseProps, Logger logger )
+        throws IOException
+    {
         if ( !propFile.exists() )
         {
             throw new FileNotFoundException( propFile.toString() );
@@ -92,7 +116,7 @@
         for ( Object o : fileProps.keySet() )
         {
             final String k = (String) o;
-            final String propValue = getPropertyValue( k, combinedProps );
+            final String propValue = getPropertyValue( k, combinedProps, logger );
             fileProps.setProperty( k, propValue );
         }
 
@@ -111,6 +135,24 @@
     public static Properties loadPropertyFile( File propfile, boolean fail, boolean useSystemProps )
         throws IOException
     {
+        return loadPropertyFile( propfile, fail, useSystemProps, null );
+    }
+
+    /**
+     * Reads a property file, resolving all internal variables.
+     *
+     * @param propfile The property file to load
+     * @param fail whether to throw an exception when the file cannot be loaded or to return null
+     * @param useSystemProps whether to incorporate System.getProperties settings into the returned Properties object.
+     * @param logger Logger instance
+     * @return the loaded and fully resolved Properties object
+     * @throws IOException if profile does not exist, or cannot be read.
+     *
+     * @since 3.1.2
+     */
+    public static Properties loadPropertyFile( File propfile, boolean fail, boolean useSystemProps, Logger logger )
+        throws IOException
+    {
 
         final Properties baseProps = new Properties();
 
@@ -122,7 +164,7 @@
         final Properties resolvedProps = new Properties();
         try
         {
-            resolvedProps.putAll( loadPropertyFile( propfile, baseProps ) );
+            resolvedProps.putAll( loadPropertyFile( propfile, baseProps, logger ) );
         }
         catch ( FileNotFoundException e )
         {
@@ -147,15 +189,21 @@
      *
      * @param k
      * @param p
+     * @param logger Logger instance
      * @return The filtered property value.
      */
-    private static String getPropertyValue( String k, Properties p )
+    private static String getPropertyValue( String k, Properties p, Logger logger )
     {
         // This can also be done using InterpolationFilterReader,
         // but it requires reparsing the file over and over until
         // it doesn't change.
 
+        // for cycle detection
+        List<String> valueChain = new LinkedList<String>();
+        valueChain.add( k );
+
         String v = p.getProperty( k );
+        String defaultValue = v;
         String ret = "";
         int idx, idx2;
 
@@ -180,26 +228,57 @@
             v = v.substring( idx2 + 1 );
             String nv = p.getProperty( nk );
 
-            // try global environment..
-            if ( nv == null && !StringUtils.isEmpty( nk ) )
+            if ( valueChain.contains( nk ) )
             {
-                nv = System.getProperty( nk );
-            }
-
-            // if the key cannot be resolved,
-            // leave it alone ( and don't parse again )
-            // else prefix the original string with the
-            // resolved property ( so it can be parsed further )
-            // taking recursion into account.
-            if ( nv == null || nv.equals( k ) || k.equals( nk ) )
-            {
-                ret += "${" + nk + "}";
+                if ( logger != null )
+                {
+                    logCircularDetection( valueChain, nk, logger );
+                }
+                return defaultValue;
             }
             else
             {
-                v = nv + v;
+                valueChain.add( nk );
+
+                // try global environment..
+                if ( nv == null && !StringUtils.isEmpty( nk ) )
+                {
+                    nv = System.getProperty( nk );
+                }
+
+                // if the key cannot be resolved,
+                // leave it alone ( and don't parse again )
+                // else prefix the original string with the
+                // resolved property ( so it can be parsed further )
+                // taking recursion into account.
+                if ( nv == null || nv.equals( k ) || k.equals( nk ) )
+                {
+                    ret += "${" + nk + "}";
+                }
+                else
+                {
+                    v = nv + v;
+                }
             }
         }
+
         return ret + v;
     }
+
+    /**
+     * Logs the detected cycle in properties resolution
+     * @param valueChain the secuence of properties resolved so fa
+     * @param nk the key the closes the cycle
+     * @param logger Logger instance
+     */
+    private static void logCircularDetection( List<String> valueChain, String nk, Logger logger )
+    {
+        StringBuilder sb = new StringBuilder( "Circular reference between properties detected: " );
+        for ( String key : valueChain )
+        {
+            sb.append( key ).append( " => " );
+        }
+        sb.append( nk );
+        logger.warn( sb.toString() );
+    }
 }
diff --git a/src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java b/src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java
index c86f72b..a9e651c 100644
--- a/src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java
+++ b/src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java
@@ -21,9 +21,12 @@
 
 import java.io.File;
 import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Properties;
 
 import org.codehaus.plexus.PlexusTestCase;
+import org.codehaus.plexus.logging.Logger;
 
 /**
  * @author Olivier Lamy
@@ -111,4 +114,199 @@
         assertEquals( "realVersion", interpolated.get( "bar" ) );
         assertEquals( "none filtered", interpolated.get( "none" ) );
     }
+    
+    /**
+     * Test case to reproduce MSHARED-417
+     * 
+     * @throws IOException if problem writing file
+     */
+    public void testCircularReferences()
+        throws IOException
+    {
+        File basicProp = new File( testDirectory, "circular.properties" );
+
+        if ( basicProp.exists() )
+        {
+            basicProp.delete();
+        }
+
+        basicProp.createNewFile();
+        FileWriter writer = new FileWriter( basicProp );
+
+        writer.write( "test=${test2}\n" );
+        writer.write( "test2=${test2}\n" );
+        writer.flush();
+        writer.close();
+
+        MockLogger logger = new MockLogger();
+
+        Properties prop = PropertyUtils.loadPropertyFile( basicProp, null, logger );
+        assertEquals( "${test2}", prop.getProperty( "test" ) );
+        assertEquals( "${test2}", prop.getProperty( "test2" ) );
+        assertEquals( 2, logger.warnMsgs.size() );
+        assertWarn( "Circular reference between properties detected: test2 => test2", logger );
+        assertWarn( "Circular reference between properties detected: test => test2 => test2", logger );
+    }
+
+    /**
+     * Test case to reproduce MSHARED-417
+     * 
+     * @throws IOException if problem writing file
+     */
+    public void testCircularReferences3Vars()
+        throws IOException
+    {
+        File basicProp = new File( testDirectory, "circular.properties" );
+
+        if ( basicProp.exists() )
+        {
+            basicProp.delete();
+        }
+
+        basicProp.createNewFile();
+        FileWriter writer = new FileWriter( basicProp );
+
+        writer.write( "test=${test2}\n" );
+        writer.write( "test2=${test3}\n" );
+        writer.write( "test3=${test}\n" );
+        writer.flush();
+        writer.close();
+
+        MockLogger logger = new MockLogger();
+
+        Properties prop = PropertyUtils.loadPropertyFile( basicProp, null, logger );
+        assertEquals( "${test2}", prop.getProperty( "test" ) );
+        assertEquals( "${test3}", prop.getProperty( "test2" ) );
+        assertEquals( "${test}", prop.getProperty( "test3" ) );
+        assertEquals( 3, logger.warnMsgs.size() );
+        assertWarn( "Circular reference between properties detected: test3 => test => test2 => test3", logger );
+        assertWarn( "Circular reference between properties detected: test2 => test3 => test => test2", logger );
+        assertWarn( "Circular reference between properties detected: test => test2 => test3 => test", logger );
+    }
+
+    private void assertWarn( String expected, MockLogger logger )
+    {
+        assertTrue( logger.warnMsgs.contains( expected ) );
+    }
+
+    private static class MockLogger
+        implements Logger
+    {
+
+        ArrayList<String> warnMsgs = new ArrayList<String>();
+
+        @Override
+        public void debug( String message )
+        {
+            // nothing
+        }
+
+        @Override
+        public void debug( String message, Throwable throwable )
+        {
+            // nothing
+        }
+
+        @Override
+        public boolean isDebugEnabled()
+        {
+            return false;
+        }
+
+        @Override
+        public void info( String message )
+        {
+            // nothing
+        }
+
+        @Override
+        public void info( String message, Throwable throwable )
+        {
+            // nothing
+        }
+
+        @Override
+        public boolean isInfoEnabled()
+        {
+            return false;
+        }
+
+        @Override
+        public void warn( String message )
+        {
+            warnMsgs.add( message );
+        }
+
+        @Override
+        public void warn( String message, Throwable throwable )
+        {
+            // nothing
+        }
+
+        @Override
+        public boolean isWarnEnabled()
+        {
+            return false;
+        }
+
+        @Override
+        public void error( String message )
+        {
+            // nothing
+        }
+
+        @Override
+        public void error( String message, Throwable throwable )
+        {
+            // nothing
+        }
+
+        @Override
+        public boolean isErrorEnabled()
+        {
+            return false;
+        }
+
+        @Override
+        public void fatalError( String message )
+        {
+            // nothing
+        }
+
+        @Override
+        public void fatalError( String message, Throwable throwable )
+        {
+            // nothing
+        }
+
+        @Override
+        public boolean isFatalErrorEnabled()
+        {
+            return false;
+        }
+
+        @Override
+        public int getThreshold()
+        {
+            return 0;
+        }
+
+        @Override
+        public void setThreshold( int threshold )
+        {
+            // nothing
+        }
+
+        @Override
+        public Logger getChildLogger( String name )
+        {
+            return null;
+        }
+
+        @Override
+        public String getName()
+        {
+            return null;
+        }
+    }
 }