From 4cdfb3f4eddb1c3ccaa304b1c8d3fade7e18f75e Mon Sep 17 00:00:00 2001 From: EA117 Date: Mon, 26 Aug 2019 21:07:01 -0500 Subject: [PATCH 1/7] [ticket/16141] plupload chunk_size incorrect when 'unlimited' is involved. Change get_chunk_size() calculation to correctly calculate limits without letting a zero "unlimited" value always win. Also ensure get_chunk_size() can only return zero if all of the limits were in fact set to unlimited. PHPBB3-16141 --- phpBB/phpbb/plupload/plupload.php | 52 ++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/phpBB/phpbb/plupload/plupload.php b/phpBB/phpbb/plupload/plupload.php index eb698fb35d..ac4811e4ef 100644 --- a/phpBB/phpbb/plupload/plupload.php +++ b/phpBB/phpbb/plupload/plupload.php @@ -283,15 +283,51 @@ class plupload */ public function get_chunk_size() { - $max = min( - $this->php_ini->getBytes('upload_max_filesize'), - $this->php_ini->getBytes('post_max_size'), - max(1, $this->php_ini->getBytes('memory_limit')), - $this->config['max_filesize'] - ); + $max = 0; + + $limit = $this->php_ini->getBytes('memory_limit'); + + // unlimited is -1 for memory_limit. 0 would be an invalid configuration. + + if ($limit > 0) + { + $max = $limit; + } + + // For all remaining limits, 0 means "unlimited". + + // For each limit, if there is a non-unlimited value to + // apply, apply the limit if it's less than whatever non- + // unlimited max value is currently set. Also, apply the + // limit if the current max value is otherwise unlimited. + + $limit = $this->php_ini->getBytes('upload_max_filesize'); + + if ($limit > 0) + { + $max = min($limit, max($max, $limit)); + } + + $limit = $this->php_ini->getBytes('post_max_size'); + + if ($limit > 0) + { + $max = min($limit, max($max, $limit)); + } + + $limit = $this->config['max_filesize']; + + if ($limit > 0) + { + $max = min($limit, max($max, $limit)); + } + + // Only if every limit was 0/unlimited will we still + // have a zero value in $max at this point. + + // Use half of the maximum possible to leave plenty of + // room for other POST data and be well under limits. - // Use half of the maximum possible to leave plenty of room for other - // POST data. return floor($max / 2); } From 73537bcc7d8b10fe77e91068d494c1181b11d6c7 Mon Sep 17 00:00:00 2001 From: EA117 Date: Wed, 28 Aug 2019 20:22:33 -0500 Subject: [PATCH 2/7] [ticket/16141] plupload chunk_size incorrect when 'unlimited' is involved. Change get_chunk_size() calculation to correctly calculate limits without letting a zero "unlimited" value always win. Also ensure get_chunk_size() can only return zero if all of the limits were in fact set to unlimited. PHPBB3-16141 --- phpBB/phpbb/plupload/plupload.php | 51 ++++++++++++------------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/phpBB/phpbb/plupload/plupload.php b/phpBB/phpbb/plupload/plupload.php index ac4811e4ef..601b3fb440 100644 --- a/phpBB/phpbb/plupload/plupload.php +++ b/phpBB/phpbb/plupload/plupload.php @@ -276,8 +276,14 @@ class plupload } /** - * Checks various php.ini values and the maximum file size to determine - * the maximum size chunks a file can be split up into for upload + * Checks various php.ini values to determine the maximum chunk + * size a file should be split into for upload. + * + * The intention is to calculate a value which reflects whatever + * the most restrictive limit is set to. And to then set the chunk + * size to half that value, to ensure any required transfer overhead + * and POST data remains well within the limit. Or, if all of the + * limits are set to unlimited, the chunk size will also be unlimited. * * @return int */ @@ -285,48 +291,31 @@ class plupload { $max = 0; - $limit = $this->php_ini->getBytes('memory_limit'); + // unlimited is -1 for memory_limit. 0 should be an invalid configuration. + $limit_memory = $this->php_ini->getBytes('memory_limit'); - // unlimited is -1 for memory_limit. 0 would be an invalid configuration. - - if ($limit > 0) + if ($limit_memory > 0) { - $max = $limit; + $max = $limit_memory; } // For all remaining limits, 0 means "unlimited". - // For each limit, if there is a non-unlimited value to - // apply, apply the limit if it's less than whatever non- - // unlimited max value is currently set. Also, apply the - // limit if the current max value is otherwise unlimited. + $limit_upload = $this->php_ini->getBytes('upload_max_filesize'); - $limit = $this->php_ini->getBytes('upload_max_filesize'); - - if ($limit > 0) + if ($limit_upload > 0) { - $max = min($limit, max($max, $limit)); + $max = min($limit_upload, $max ? $max : $limit_upload); } - $limit = $this->php_ini->getBytes('post_max_size'); + $limit_post = $this->php_ini->getBytes('post_max_size'); - if ($limit > 0) + if ($limit_post > 0) { - $max = min($limit, max($max, $limit)); + $max = min($limit_post, $max ? $max : $limit_post); } - - $limit = $this->config['max_filesize']; - - if ($limit > 0) - { - $max = min($limit, max($max, $limit)); - } - - // Only if every limit was 0/unlimited will we still - // have a zero value in $max at this point. - - // Use half of the maximum possible to leave plenty of - // room for other POST data and be well under limits. + + // $config['max_filesize'] is not a limiter to chunk size. return floor($max / 2); } From 29d43670430f3edad2366ddbca15d1b34315ce1d Mon Sep 17 00:00:00 2001 From: EA117 Date: Wed, 28 Aug 2019 23:34:52 -0500 Subject: [PATCH 3/7] [ticket/16141] plupload chunk_size when 'unlimited' is involved. Change get_chunk_size() calculation to correctly calculate limits without letting a zero "unlimited" value always win. Also ensure get_chunk_size() can only return zero if all of the limits were in fact set to unlimited. PHPBB3-16141 --- phpBB/phpbb/plupload/plupload.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/plupload/plupload.php b/phpBB/phpbb/plupload/plupload.php index 601b3fb440..f9403938ed 100644 --- a/phpBB/phpbb/plupload/plupload.php +++ b/phpBB/phpbb/plupload/plupload.php @@ -305,14 +305,14 @@ class plupload if ($limit_upload > 0) { - $max = min($limit_upload, $max ? $max : $limit_upload); + $max = min($limit_upload, ($max ? $max : $limit_upload)); } $limit_post = $this->php_ini->getBytes('post_max_size'); if ($limit_post > 0) { - $max = min($limit_post, $max ? $max : $limit_post); + $max = min($limit_post, ($max ? $max : $limit_post)); } // $config['max_filesize'] is not a limiter to chunk size. From bf359d153dd0ff6cc9505cdd7bf8a7754b6a6073 Mon Sep 17 00:00:00 2001 From: EA117 Date: Thu, 29 Aug 2019 00:17:14 -0500 Subject: [PATCH 4/7] [ticket/16141] plupload chunk_size when 'unlimited' is involved. Change get_chunk_size() calculation to correctly calculate limits without letting a zero "unlimited" value always win. Also ensure get_chunk_size() can only return zero if all of the limits were in fact set to unlimited. PHPBB3-16141 --- phpBB/phpbb/plupload/plupload.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/plupload/plupload.php b/phpBB/phpbb/plupload/plupload.php index f9403938ed..91b635b617 100644 --- a/phpBB/phpbb/plupload/plupload.php +++ b/phpBB/phpbb/plupload/plupload.php @@ -314,7 +314,7 @@ class plupload { $max = min($limit_post, ($max ? $max : $limit_post)); } - + // $config['max_filesize'] is not a limiter to chunk size. return floor($max / 2); From 5bd3b7ec378579dc84d2d838ba43d3a77f519159 Mon Sep 17 00:00:00 2001 From: EA117 Date: Fri, 30 Aug 2019 07:01:04 -0500 Subject: [PATCH 5/7] [ticket/16141] plupload chunk_size when 'unlimited' is involved. Change get_chunk_size() calculation to correctly calculate limits without letting a zero "unlimited" value always win. Also ensure get_chunk_size() can only return zero if all of the limits were in fact set to unlimited. PHPBB3-16141 --- phpBB/phpbb/plupload/plupload.php | 56 +++++++++++++------------------ 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/phpBB/phpbb/plupload/plupload.php b/phpBB/phpbb/plupload/plupload.php index 91b635b617..70070b9835 100644 --- a/phpBB/phpbb/plupload/plupload.php +++ b/phpBB/phpbb/plupload/plupload.php @@ -276,47 +276,37 @@ class plupload } /** - * Checks various php.ini values to determine the maximum chunk - * size a file should be split into for upload. - * - * The intention is to calculate a value which reflects whatever - * the most restrictive limit is set to. And to then set the chunk - * size to half that value, to ensure any required transfer overhead - * and POST data remains well within the limit. Or, if all of the - * limits are set to unlimited, the chunk size will also be unlimited. - * - * @return int - */ + * Checks various php.ini values to determine the maximum chunk + * size a file should be split into for upload. + * + * The intention is to calculate a value which reflects whatever + * the most restrictive limit is set to. And to then set the chunk + * size to half that value, to ensure any required transfer overhead + * and POST data remains well within the limit. Or, if all of the + * limits are set to unlimited, the chunk size will also be unlimited. + * + * @return int + * + * @access public + */ public function get_chunk_size() { $max = 0; - // unlimited is -1 for memory_limit. 0 should be an invalid configuration. - $limit_memory = $this->php_ini->getBytes('memory_limit'); + $limits = [ + $this->php_ini->getBytes('memory_limit'), + $this->php_ini->getBytes('upload_max_filesize'), + $this->php_ini->getBytes('post_max_size'), + ]; - if ($limit_memory > 0) + foreach ($limits as $limit_type) { - $max = $limit_memory; + if ($limit_type > 0) + { + $max = ($max !== 0) ? min($limit_type, $max) : $limit_type; + } } - // For all remaining limits, 0 means "unlimited". - - $limit_upload = $this->php_ini->getBytes('upload_max_filesize'); - - if ($limit_upload > 0) - { - $max = min($limit_upload, ($max ? $max : $limit_upload)); - } - - $limit_post = $this->php_ini->getBytes('post_max_size'); - - if ($limit_post > 0) - { - $max = min($limit_post, ($max ? $max : $limit_post)); - } - - // $config['max_filesize'] is not a limiter to chunk size. - return floor($max / 2); } From b149e50bf16850cab268660c0f7291be6c130f9f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 19 Oct 2019 16:11:22 +0200 Subject: [PATCH 6/7] [ticket/16141] Add tests for plupload's get_chunk_size() PHPBB3-16141 --- tests/plupload/plupload_test.php | 68 ++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/plupload/plupload_test.php b/tests/plupload/plupload_test.php index 46bebb8d35..65141f4f2a 100644 --- a/tests/plupload/plupload_test.php +++ b/tests/plupload/plupload_test.php @@ -54,4 +54,72 @@ class phpbb_plupload_test extends phpbb_test_case $this->assertEquals($expected, $plupload->generate_resize_string()); } + + public function data_get_chunk_size() + { + return [ + [[ + 'memory_limit' => -1, + 'upload_max_filesize' => 0, + 'post_max_size' => 0, + ], 0], + [[ + 'memory_limit' => -1, + 'upload_max_filesize' => 500, + 'post_max_size' => 400, + ], 200], + [[ + 'memory_limit' => 100, + 'upload_max_filesize' => 0, + 'post_max_size' => 300, + ], 50], + [[ + 'memory_limit' => 300, + 'upload_max_filesize' => 200, + 'post_max_size' => 0, + ], 100], + [[ + 'memory_limit' => 3000, + 'upload_max_filesize' => 800, + 'post_max_size' => 900, + ], 400], + [[ + 'memory_limit' => 2000, + 'upload_max_filesize' => 1000, + 'post_max_size' => 600, + ], 300], + ]; + } + + /** + * @dataProvider data_get_chunk_size + */ + public function test_get_chunk_size($limits_ary, $expected) + { + global $phpbb_root_path, $phpEx; + + $lang = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx)); + $config = new \phpbb\config\config([]); + + $ini_wrapper = $this->getMockBuilder('\bantu\IniGetWrapper\IniGetWrapper') + ->setMethods(['getBytes']) + ->getMock(); + $ini_wrapper->method('getBytes') + ->will($this->returnValueMap([ + ['memory_limit', $limits_ary['memory_limit']], + ['upload_max_filesize', $limits_ary['upload_max_filesize']], + ['post_max_size', $limits_ary['post_max_size']] + ])); + + $plupload = new \phpbb\plupload\plupload( + '', + $config, + new phpbb_mock_request, + new \phpbb\user($lang, '\phpbb\datetime'), + $ini_wrapper, + new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)) + ); + + $this->assertEquals($expected, $plupload->get_chunk_size()); + } } From 6eb00414536f583e4e1ac5fc72cb00025f55b2ce Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 19 Oct 2019 17:35:25 +0200 Subject: [PATCH 7/7] [ticket/16141] Add assertion for memory limit divided by two PHPBB3-16141 --- tests/plupload/plupload_test.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/plupload/plupload_test.php b/tests/plupload/plupload_test.php index 65141f4f2a..eb4657afbc 100644 --- a/tests/plupload/plupload_test.php +++ b/tests/plupload/plupload_test.php @@ -88,6 +88,11 @@ class phpbb_plupload_test extends phpbb_test_case 'upload_max_filesize' => 1000, 'post_max_size' => 600, ], 300], + [[ + 'memory_limit' => 1000, + 'upload_max_filesize' => 2000, + 'post_max_size' => 3000, + ], 500], ]; }