Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for undetected circular definitions when resolving properties. #249

Merged
merged 6 commits into from

2 participants

@HuffAndPuff

PropertyTask::resolveAllProperties checks for circularly defined properties by comparing the property being resolved against each new one added. This way, a circular chain like in this .properties file is detected:

A=${B}
B=${A}

However it fails to detect this one:

A=${B}
B=${C}
C=${B}

It will forever check A==B and A==C without ever breaking out.

With this commit, it checks against all properties used to resolve the current one. I'm almost sure this will detect any circular definition. It fixes the hangs I found anyway.

HuffAndPuff added some commits
@HuffAndPuff HuffAndPuff Added HangDetectorPropertyTask to the PropertyTask tests.
I want to add some tests that currently fail to detect a circular
reference, but running these tests (obviously) causes PHPUnit to
hang which is very unpleasant.

HangDetectorPropertyTask extends PropertyTask and uses a custom
HangDetectorProperties (extends Properties) that counts the number
of accesses. More than 100 is interpreted as an undetected circular
reference. Don't use big property sets when testing...
ca48771
@HuffAndPuff HuffAndPuff Changed test3 to testCircularReferenceDetection and added data provider. c602d47
@HuffAndPuff HuffAndPuff Add two new PropertyTask tests where circular definition detection fa…
…ils.

Below, A->B means resolves to B in some way, like A=xx${B}yy

Currently a chain like A->B->A will be detected in the
resolveAllProperties() function since we check each new property in the
chain against the property were trying to resolve.

This fails if we start to resolve A but the circular chain doesn't
involve A:
A->B->C->B->C->B....

These two tests reflect that.
e83854a
@HuffAndPuff HuffAndPuff For consistence, it's circular "Definition", not "Reference". 6fc0be1
@HuffAndPuff HuffAndPuff Fix for the tests testCircularDefinition1 and testCircularDefinition2.
In resolveAllProperties() we remember which properties from $props have
been used to resolve the current property. If the same property appears
twice, we have a circular property definition.

This will detect chains like A resolves to B resolves to C resolves to B...
742db17
@HuffAndPuff HuffAndPuff Forgot to add file... 200cfbe
@HuffAndPuff

Dunno why the test fails, seems unrelated to my changes.

Anyways, ProjectConfiguratior::replaceProperties could use a similar mechanism to detect infinite loops instead of the current limit 5 of iterations. There you can either throw an error or log a warning.

Is there an specific reason for silently ignoring infinite loops like this or is a something like this desired? I could add it if requested.

@mrook mrook merged commit 002958f into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 22, 2013
  1. @HuffAndPuff

    Added HangDetectorPropertyTask to the PropertyTask tests.

    HuffAndPuff authored
    I want to add some tests that currently fail to detect a circular
    reference, but running these tests (obviously) causes PHPUnit to
    hang which is very unpleasant.
    
    HangDetectorPropertyTask extends PropertyTask and uses a custom
    HangDetectorProperties (extends Properties) that counts the number
    of accesses. More than 100 is interpreted as an undetected circular
    reference. Don't use big property sets when testing...
  2. @HuffAndPuff
  3. @HuffAndPuff

    Add two new PropertyTask tests where circular definition detection fa…

    HuffAndPuff authored
    …ils.
    
    Below, A->B means resolves to B in some way, like A=xx${B}yy
    
    Currently a chain like A->B->A will be detected in the
    resolveAllProperties() function since we check each new property in the
    chain against the property were trying to resolve.
    
    This fails if we start to resolve A but the circular chain doesn't
    involve A:
    A->B->C->B->C->B....
    
    These two tests reflect that.
  4. @HuffAndPuff
  5. @HuffAndPuff

    Fix for the tests testCircularDefinition1 and testCircularDefinition2.

    HuffAndPuff authored
    In resolveAllProperties() we remember which properties from $props have
    been used to resolve the current property. If the same property appears
    twice, we have a circular property definition.
    
    This will detect chains like A resolves to B resolves to C resolves to B...
  6. @HuffAndPuff

    Forgot to add file...

    HuffAndPuff authored
This page is out of date. Refresh to see the latest.
View
7 classes/phing/tasks/system/PropertyTask.php
@@ -355,7 +355,7 @@ protected function resolveAllProperties(Properties $props) {
$name = array_shift($keys);
$value = $props->getProperty($name);
$resolved = false;
-
+ $resolveStack = array();
while(!$resolved) {
$fragments = array();
@@ -376,16 +376,17 @@ protected function resolveAllProperties(Properties $props) {
if ($fragment === null) {
$propertyName = array_shift($j);
- if ($propertyName === $name) {
+ if( in_array($propertyName, $resolveStack) ) {
// Should we maybe just log this as an error & move on?
// $this->log("Property ".$name." was circularly defined.", Project::MSG_ERR);
- throw new BuildException("Property ".$name." was circularly defined.");
+ throw new BuildException("Property ".$propertyName." was circularly defined.");
}
$fragment = $this->getProject()->getProperty($propertyName);
if ($fragment === null) {
if ($props->containsKey($propertyName)) {
$fragment = $props->getProperty($propertyName);
+ $resolveStack[] = $propertyName;
$resolved = false; // parse again (could have been replaced w/ another var)
} else {
$fragment = "\${".$propertyName."}";
View
57 test/classes/phing/tasks/PropertyTaskTest.php
@@ -21,6 +21,7 @@
*/
require_once 'phing/BuildFileTest.php';
+require_once 'phing/tasks/system/PropertyTask.php';
/**
* @author Hans Lellelid (Phing)
@@ -43,17 +44,6 @@ public function test2() {
$this->expectLog("test2", "testprop1=aa, testprop3=xxyy, testprop4=aazz");
}
- public function test3() {
- try {
- $this->executeTarget("test3");
- } catch (BuildException $e) {
- $this->assertTrue(strpos($e->getMessage(), "was circularly defined") !== false, "Circular definition not detected - ");
- return;
- }
- $this->fail("Did not throw exception on circular exception");
- }
-
-
public function test4() {
$this->expectLog("test4", "http.url is http://localhost:999");
}
@@ -79,5 +69,48 @@ public function testFilterChain()
$this->executeTarget(__FUNCTION__);
$this->assertEquals("World", $this->project->getProperty("filterchain.test"));
}
-
+
+ public function circularDefinitionTargets()
+ {
+ return array(
+ array('test3'),
+ array('testCircularDefinition1'),
+ array('testCircularDefinition2')
+ );
+ }
+
+ /**
+ * @dataProvider circularDefinitionTargets
+ */
+ public function testCircularDefinitionDetection($target)
+ {
+ try {
+ $this->executeTarget($target);
+ } catch (BuildException $e) {
+ $this->assertTrue(strpos($e->getMessage(), "was circularly defined") !== false, "Circular definition not detected - ");
+ return;
+ }
+ $this->fail("Did not throw exception on circular exception");
+ }
+}
+
+class HangDetectorPropertyTask extends PropertyTask {
+
+ protected function loadFile(PhingFile $file) {
+ $props = new HangDetectorProperties();
+ $props->load($file);
+ $this->addProperties($props);
+ }
+}
+
+class HangDetectorProperties extends Properties {
+ private $accesses = 0;
+ public function getProperty($prop)
+ {
+ $this->accesses++;
+ if( $this->accesses > 100 ) {
+ throw new Exception('Cirular definition Hanged!');
+ }
+ return parent::getProperty($prop);
+ }
}
View
9 test/etc/tasks/property.xml
@@ -40,4 +40,13 @@
</property>
</target>
+ <taskdef name="hangdetectorproperty" classname="HangDetectorPropertyTask" />
+ <target name="testCircularDefinition1">
+ <property name="testprop2" value="${testprop1}" />
+ <hangdetectorproperty file="property2.properties"/>
+ </target>
+
+ <target name="testCircularDefinition2">
+ <hangdetectorproperty file="property_hang.properties"/>
+ </target>
</project>
View
5 test/etc/tasks/property_hang.properties
@@ -0,0 +1,5 @@
+testprop1=xx${testprop2}yy
+testprop2=aa${testprop3}bb
+testprop3=cc${testprop2}dd
+
+
Something went wrong with that request. Please try again.