Avoid setting memory_limit to lower value than the one already configured (#215)

Some scripts are setting the memory_limit PHP option : setup, csvimport and XLSX export. This was done to avoid crashing when dealing with such large amount of data.
But sometimes we were setting the value without any prior check, so we could actually lower the memory_limit value :/

Now this memory_limit change is done using \utils::SetMinMemoryLimit, which will call ini_set if and only if the current value is lower than the one to be set.

Setup calls (setup/ajax.dataloader.php and webservices/backoffice.dataloader.php) were left as is as they weren't subject to this bug, and also they are more complex (logging done on each case).

application/utils.inc.php

@@ -563,48 +563,86 @@ class utils
 
 	public static function ReadFromFile($sFileName)
 	{
-		if (!file_exists($sFileName)) return false;
+		if (!file_exists($sFileName)) {
+			return false;
+		}
+
 		return file_get_contents($sFileName);
 	}
 
 	/**
 	 * Helper function to convert a value expressed in a 'user friendly format'
 	 * as in php.ini, e.g. 256k, 2M, 1G etc. Into a number of bytes
+	 *
 	 * @param mixed $value The value as read from php.ini
+	 *
 	 * @return number
 	 */
-	public static function ConvertToBytes( $value )
+	public static function ConvertToBytes($value)
 	{
 		$iReturn = $value;
-	    if ( !is_numeric( $value ) )
-		{
-	        $iLength = strlen( $value );
-	        $iReturn = substr( $value, 0, $iLength - 1 );
-	        $sUnit = strtoupper( substr( $value, $iLength - 1 ) );
-	        switch ( $sUnit )
-			{
-	            case 'G':
-	                $iReturn *= 1024;
-	            case 'M':
-	                $iReturn *= 1024;
-	            case 'K':
-	                $iReturn *= 1024;
-	        }
-	    }
-        return $iReturn;
-    }
-  
-  /**
-   * Checks if the memory limit is at least what is required
-   *
-   * @param int $memoryLimit set limit in bytes
-   * @param int $requiredLimit required limit in bytes
-   * @return bool
-   */
-  public static function IsMemoryLimitOk($memoryLimit, $requiredLimit)
-  {
-      return ($memoryLimit >= $requiredLimit) || ($memoryLimit == -1);
-  }
+		if (!is_numeric($value)) {
+			$iLength = strlen($value);
+			$iReturn = substr($value, 0, $iLength - 1);
+			$sUnit = strtoupper(substr($value, $iLength - 1));
+			switch ($sUnit) {
+				case 'G':
+					$iReturn *= 1024;
+				case 'M':
+					$iReturn *= 1024;
+				case 'K':
+					$iReturn *= 1024;
+			}
+		}
+
+		return $iReturn;
+	}
+
+	/**
+	 * Checks if the memory limit is at least what is required
+	 *
+	 * @param int $iMemoryLimit set limit in bytes
+	 * @param int $iRequiredLimit required limit in bytes
+	 *
+	 * @return bool
+	 */
+	public static function IsMemoryLimitOk($iMemoryLimit, $iRequiredLimit)
+	{
+		return ($iMemoryLimit >= $iRequiredLimit) || ($iMemoryLimit === -1);
+	}
+
+	/**
+	 * Set memory_limit to required value
+	 *
+	 * @param string $sRequiredLimit required limit, for example '512M'
+	 *
+	 * @return bool|null null if nothing was done, true if modifying memory_limit was successful, false otherwise
+	 *
+	 * @uses utils::ConvertToBytes()
+	 * @uses \ini_get('memory_limit')
+	 * @uses \ini_set()
+	 * @uses utils::ConvertToBytes()
+	 *
+	 * @since 2.7.5 N°3806
+	 */
+	public static function SetMinMemoryLimit($sRequiredLimit)
+	{
+		$iRequiredLimit = static::ConvertToBytes($sRequiredLimit);
+		$sMemoryLimit = trim(ini_get('memory_limit'));
+		if (empty($sMemoryLimit)) {
+			// On some PHP installations, memory_limit does not exist as a PHP setting!
+			// (encountered on a 5.2.0 under Windows)
+			// In that case, ini_set will not work
+			return false;
+		}
+		$iMemoryLimit = static::ConvertToBytes($sMemoryLimit);
+
+		if (static::IsMemoryLimitOk($iMemoryLimit, $iRequiredLimit)) {
+			return null;
+		}
+
+		return ini_set('memory_limit', $iRequiredLimit);
+	}
 
 	/**
 	 * Format a value into a more friendly format (KB, MB, GB, TB) instead a juste a Bytes amount.

pages/ajax.render.php

@@ -2060,7 +2060,9 @@ EOF
 
 		case 'xlsx_run':
 			$sMemoryLimit = MetaModel::GetConfig()->Get('xlsx_exporter_memory_limit');
-			ini_set('memory_limit', $sMemoryLimit);
+			if (utils::SetMinMemoryLimit($sMemoryLimit) === false) {
+				IssueLog::Warning("XSLX export : cannot set memory_limit to {$sMemoryLimit}");
+			}
 			ini_set('max_execution_time', max(300, ini_get('max_execution_time'))); // At least 5 minutes
 
 			$sToken = utils::ReadParam('token', '', false, 'raw_data');
@@ -2068,8 +2070,7 @@ EOF
 			$oExcelExporter = new ExcelExporter($sToken);
 			$aStatus = $oExcelExporter->Run();
 			$aResults = array('status' => $aStatus['code'], 'percentage' => $aStatus['percentage'], 'message' => $aStatus['message']);
-			if ($aStatus['code'] == 'done')
-			{
+			if ($aStatus['code'] == 'done') {
 				$aResults['statistics'] = $oExcelExporter->GetStatistics('html');
 			}
 			$oPage->add(json_encode($aResults));

pages/csvimport.php

@@ -17,21 +17,24 @@
  * You should have received a copy of the GNU Affero General Public License
  */
 
-try
-{
-	ini_set('memory_limit', '256M');
+try {
 	require_once('../approot.inc.php');
 	require_once(APPROOT.'/application/application.inc.php');
 	require_once(APPROOT.'/application/itopwebpage.class.inc.php');
 	require_once(APPROOT.'/application/ajaxwebpage.class.inc.php');
-	
+
 	require_once(APPROOT.'/application/startup.inc.php');
-	
+
 	require_once(APPROOT.'/application/loginwebpage.class.inc.php');
+
+	if (utils::SetMinMemoryLimit('256M') === false) {
+		IssueLog::Warning('csvimport : cannot set minimum memory_limit !');
+	}
+
 	LoginWebPage::DoLogin(); // Check user rights and prompt if needed
 	
 	$iStep = utils::ReadParam('step', 1);
-	
+
 	$oPage = new iTopWebPage(Dict::S('UI:Title:BulkImport'));
 	$oPage->SetBreadCrumbEntry('ui-tool-bulkimport', Dict::S('Menu:CSVImportMenu'), Dict::S('UI:Title:BulkImport+'), '', utils::GetAbsoluteUrlAppRoot().'images/wrench.png');
 
@@ -1542,4 +1545,3 @@ catch(Exception $e)
 		IssueLog::Error($e->getMessage());
 	}
 }
-?>

test/application/UtilsTest.php

@@ -426,4 +426,26 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase
 			],
 		];
 	}
+
+	/**
+	 * @param string $sExpressionToConvert
+	 * @param int $iExpectedConvertedValue
+	 *
+	 * @dataProvider ConvertToBytesProvider
+	 */
+	public function testConvertToBytes($sExpressionToConvert, $iExpectedConvertedValue)
+	{
+		$iCurrentConvertedValue = utils::ConvertToBytes($sExpressionToConvert);
+		self::assertEquals($iExpectedConvertedValue, $iCurrentConvertedValue, 'Converted value wasn\'t the one expected !');
+	}
+
+	public function ConvertToBytesProvider()
+	{
+		return [
+			'123' => ['123', 123],
+			'56k' => ['56k', 56 * 1024],
+			'512M' => ['512M', 512 * 1024 * 1024],
+			'2G' => ['2G', 2 * 1024 * 1024 * 1024],
+		];
+	}
 }