bz2: refactor filter creation to do parameter validation

ext/bz2/bz2_filter.c

@@ -375,22 +375,8 @@ static const php_stream_filter_ops php_bz2_compress_ops = {
 
 /* }}} */
 
-/* {{{ bzip2.* common factory */
-
-static php_stream_filter *php_bz2_filter_create(const char *filtername, zval *filterparams, bool persistent)
-{
-	const php_stream_filter_ops *fops = NULL;
-	php_stream_filter_seekable_t write_seekable;
-	php_bz2_filter_data *data;
-	int status = BZ_OK;
-
-	if (php_stream_filter_parse_write_seek_mode(filterparams, &write_seekable) == FAILURE) {
-		return NULL;
-	}
-
-	/* Create this filter */
-	data = pecalloc(1, sizeof(php_bz2_filter_data), persistent);
-
+static php_bz2_filter_data *php_bz2_filter_data_new(bool persistent) {
+	php_bz2_filter_data *data = pecalloc(1, sizeof(php_bz2_filter_data), persistent);
 	/* Circular reference */
 	data->strm.opaque = (void *) data;
 
@@ -401,86 +387,143 @@ static php_stream_filter *php_bz2_filter_create(const char *filtername, zval *fi
 	data->strm.next_in = data->inbuf = (char *) pemalloc(data->inbuf_len, persistent);
 	data->strm.avail_in = 0;
 	data->strm.next_out = data->outbuf = (char *) pemalloc(data->outbuf_len, persistent);
+	return data;
+}
 
-	if (strcasecmp(filtername, "bzip2.decompress") == 0) {
-		data->small_footprint = 0;
-		data->expect_concatenated = 0;
-
-		if (filterparams) {
-			zval *tmpzval = NULL;
+static php_stream_filter *php_bz2_decompress_filter_create(zval *filter_params, bool persistent) {
+	php_stream_filter_seekable_t write_seekable = PSFS_SEEKABLE_ALWAYS;
+	bool small_footprint = false;
+	bool expect_concatenated = false;
+
+	if (filter_params) {
+		if (UNEXPECTED(
+			Z_TYPE_P(filter_params) != IS_TRUE
+			&& Z_TYPE_P(filter_params) != IS_FALSE
+			&& Z_TYPE_P(filter_params) != IS_ARRAY
+			&& Z_TYPE_P(filter_params) != IS_OBJECT
+		)) {
+			php_error_docref(NULL, E_WARNING,
+				"Filter parameters for bzip2.decompress filter must be of type array|object|bool, %s given",
+				zend_zval_type_name(filter_params)
+			);
+			return NULL;
+		}
 
-			if (Z_TYPE_P(filterparams) == IS_ARRAY || Z_TYPE_P(filterparams) == IS_OBJECT) {
-				HashTable *ht = HASH_OF(filterparams);
+		if (Z_TYPE_P(filter_params) == IS_TRUE || Z_TYPE_P(filter_params) == IS_FALSE) {
+			small_footprint = Z_TYPE_P(filter_params) == IS_TRUE;
+		} else {
+			ZEND_ASSERT(Z_TYPE_P(filter_params) == IS_ARRAY || Z_TYPE_P(filter_params) == IS_OBJECT);
 
-				if ((tmpzval = zend_hash_str_find_ind(ht, "concatenated", sizeof("concatenated")-1))) {
-					data->expect_concatenated = zend_is_true(tmpzval);
-					tmpzval = NULL;
-				}
+			const HashTable *filter_params_ht = HASH_OF(filter_params);
+			/* TODO: convert php_stream_filter_parse_write_seek_mode() to take HashTable */
+			if (php_stream_filter_parse_write_seek_mode(filter_params, &write_seekable) == FAILURE) {
+				return NULL;
+			}
 
-				tmpzval = zend_hash_str_find_ind(ht, "small", sizeof("small")-1);
-			} else {
-				tmpzval = filterparams;
+			const zval *concatenated = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("concatenated"));
+			if (concatenated) {
+				expect_concatenated = zend_is_true(concatenated);
 			}
 
-			if (tmpzval) {
-				data->small_footprint = zend_is_true(tmpzval);
+			const zval *small = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("small"));
+			if (small) {
+				small_footprint = zend_is_true(small);
 			}
 		}
+	}
 
-		data->status = PHP_BZ2_UNINITIALIZED;
-		fops = &php_bz2_decompress_ops;
-	} else if (strcasecmp(filtername, "bzip2.compress") == 0) {
-		int blockSize100k = PHP_BZ2_FILTER_DEFAULT_BLOCKSIZE;
-		int workFactor = PHP_BZ2_FILTER_DEFAULT_WORKFACTOR;
-
-		if (filterparams) {
-			zval *tmpzval;
-
-			if (Z_TYPE_P(filterparams) == IS_ARRAY || Z_TYPE_P(filterparams) == IS_OBJECT) {
-				HashTable *ht = HASH_OF(filterparams);
-
-				if ((tmpzval = zend_hash_str_find_ind(ht, "blocks", sizeof("blocks")-1))) {
-					/* How much memory to allocate (1 - 9) x 100kb */
-					zend_long blocks = zval_get_long(tmpzval);
-					if (blocks < 1 || blocks > 9) {
-						php_error_docref(NULL, E_WARNING, "Invalid parameter given for number of blocks to allocate (" ZEND_LONG_FMT ")", blocks);
-					} else {
-						blockSize100k = (int) blocks;
-					}
-				}
+	php_bz2_filter_data *data = php_bz2_filter_data_new(persistent);
+	/* Save configuration for reset */
+	data->small_footprint = small_footprint;
+	data->expect_concatenated = expect_concatenated;
+	data->status = PHP_BZ2_UNINITIALIZED;
 
-				if ((tmpzval = zend_hash_str_find_ind(ht, "work", sizeof("work")-1))) {
-					/* Work Factor (0 - 250) */
-					zend_long work = zval_get_long(tmpzval);
-					if (work < 0 || work > 250) {
-						php_error_docref(NULL, E_WARNING, "Invalid parameter given for work factor (" ZEND_LONG_FMT ")", work);
-					} else {
-						workFactor = (int) work;
-					}
-				}
-			}
+	return php_stream_filter_alloc(&php_bz2_decompress_ops, data, persistent, PSFS_SEEKABLE_START, write_seekable);
+}
+
+static php_stream_filter *php_bz2_compress_filter_create(zval *filter_params, bool persistent) {
+	php_stream_filter_seekable_t write_seekable = PSFS_SEEKABLE_ALWAYS;
+	int blockSize100k = PHP_BZ2_FILTER_DEFAULT_BLOCKSIZE;
+	int workFactor = PHP_BZ2_FILTER_DEFAULT_WORKFACTOR;
+
+	if (filter_params) {
+		if (UNEXPECTED(Z_TYPE_P(filter_params) != IS_ARRAY && Z_TYPE_P(filter_params) != IS_OBJECT)) {
+			php_error_docref(NULL, E_WARNING,
+				"Filter parameters for bzip2.compress filter must be of type array|object, %s given",
+				zend_zval_type_name(filter_params)
+			);
+			return NULL;
 		}
 
-		/* Save configuration for reset */
-		data->blockSize100k = blockSize100k;
-		data->workFactor = workFactor;
+		const HashTable *filter_params_ht = HASH_OF(filter_params);
+		/* TODO: convert php_stream_filter_parse_write_seek_mode() to take HashTable */
+		if (php_stream_filter_parse_write_seek_mode(filter_params, &write_seekable) == FAILURE) {
+			return NULL;
+		}
 
-		status = BZ2_bzCompressInit(&(data->strm), blockSize100k, 0, workFactor);
-		data->is_flushed = 1;
-		fops = &php_bz2_compress_ops;
-	} else {
-		status = BZ_DATA_ERROR;
+		const zval *blocks_zv = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("blocks"));
+		if (blocks_zv) {
+			ZEND_ASSERT(Z_TYPE_P(blocks_zv) != IS_INDIRECT);
+			bool failed = false;
+			/* How much memory to allocate (1 - 9) x 100kb */
+			zend_long blocks = zval_try_get_long(blocks_zv, &failed);
+			if (UNEXPECTED(failed)) {
+				php_error_docref(NULL, E_WARNING, "Number of blocks parameter must be of type int, %s given", zend_zval_type_name(blocks_zv));
+				return NULL;
+			} else if (blocks < 1 || blocks > 9) {
+				php_error_docref(NULL, E_WARNING, "Number of blocks to allocate must be between 1 and 9, " ZEND_LONG_FMT " given", blocks);
+				return NULL;
+			} else {
+				blockSize100k = (int) blocks;
+			}
+		}
+
+		const zval *work_zv = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("work"));
+		if (work_zv) {
+			ZEND_ASSERT(Z_TYPE_P(work_zv) != IS_INDIRECT);
+			bool failed = false;
+			/* Work Factor (0 - 250) */
+			zend_long work = zval_try_get_long(work_zv, &failed);
+			if (UNEXPECTED(failed)) {
+				php_error_docref(NULL, E_WARNING, "Work factor parameter must be of type int, %s given", zend_zval_type_name(work_zv));
+				return NULL;
+			} else if (work < 0 || work > 250) {
+				php_error_docref(NULL, E_WARNING, "Work factor must be between 0 and 250, " ZEND_LONG_FMT " given", work);
+				return NULL;
+			} else {
+				workFactor = (int) work;
+			}
+		}
 	}
 
-	if (status != BZ_OK) {
+	php_bz2_filter_data *data = php_bz2_filter_data_new(persistent);
+	/* Save configuration for reset */
+	data->blockSize100k = blockSize100k;
+	data->workFactor = workFactor;
+
+	int status = BZ2_bzCompressInit(&(data->strm), blockSize100k, 0, workFactor);
+	if (UNEXPECTED(status != BZ_OK)) {
 		/* Unspecified (probably strm) error, let stream-filter error do its own whining */
 		pefree(data->strm.next_in, persistent);
 		pefree(data->strm.next_out, persistent);
 		pefree(data, persistent);
 		return NULL;
 	}
+	data->is_flushed = true;
+
+	return php_stream_filter_alloc(&php_bz2_compress_ops, data, persistent, PSFS_SEEKABLE_START, write_seekable);
+}
 
-	return php_stream_filter_alloc(fops, data, persistent, PSFS_SEEKABLE_START, write_seekable);
+/* {{{ bzip2.* common factory */
+static php_stream_filter *php_bz2_filter_create(const char *filtername, zval *filterparams, bool persistent)
+{
+	if (strcasecmp(filtername, "bzip2.decompress") == 0) {
+		return php_bz2_decompress_filter_create(filterparams, persistent);
+	} else if (strcasecmp(filtername, "bzip2.compress") == 0) {
+		return php_bz2_compress_filter_create(filterparams, persistent);
+	} else {
+		return NULL;
+	}
 }
 
 const php_stream_filter_factory php_bz2_filter_factory = {

ext/bz2/tests/bug72447.phpt

@@ -16,4 +16,6 @@ fclose($fp);
 unlink('testfile');
 ?>
 --EXPECTF--
-Warning: stream_filter_append(): Invalid parameter given for number of blocks to allocate (0) in %s%ebug72447.php on line %d
+Warning: stream_filter_append(): Number of blocks parameter must be of type int, string given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d

ext/bz2/tests/bz2_filter_compress_errors.phpt

@@ -0,0 +1,67 @@
+--TEST--
+bzip2.compress filter param errors
+--EXTENSIONS--
+bz2
+--FILE--
+<?php
+$fp = fopen('php://stdout', 'w');
+
+$param = 'not an array';
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['blocks' => 'not an int'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['blocks' => '15'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['blocks' => '0'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['work' => 'not an int'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['work' => '251'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+$param = ['work' => '-1'];
+var_dump(stream_filter_append($fp, 'bzip2.compress', STREAM_FILTER_WRITE, $param));
+
+fclose($fp);
+
+?>
+--EXPECTF--
+Warning: stream_filter_append(): Filter parameters for bzip2.compress filter must be of type array|object, string given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Number of blocks parameter must be of type int, string given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Number of blocks to allocate must be between 1 and 9, 15 given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Number of blocks to allocate must be between 1 and 9, 0 given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Work factor parameter must be of type int, string given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Work factor must be between 0 and 250, 251 given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)
+
+Warning: stream_filter_append(): Work factor must be between 0 and 250, -1 given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d
+bool(false)

ext/bz2/tests/bz2_filter_decompress_errors.phpt

@@ -0,0 +1,19 @@
+--TEST--
+bzip2.decompress filter param errors
+--EXTENSIONS--
+bz2
+--FILE--
+<?php
+$fp = fopen('php://stdout', 'w');
+
+$param = 'not an array or bool';
+var_dump(stream_filter_append($fp, 'bzip2.decompress', STREAM_FILTER_WRITE, $param));
+
+fclose($fp);
+
+?>
+--EXPECTF--
+Warning: stream_filter_append(): Filter parameters for bzip2.decompress filter must be of type array|object|bool, string given in %s on line %d
+
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.decompress" in %s on line %d
+bool(false)

ext/bz2/tests/bz2_filter_unknown_errors.phpt

@@ -0,0 +1,16 @@
+--TEST--
+bzip2 filter error on unknown filter name
+--EXTENSIONS--
+bz2
+--FILE--
+<?php
+$fp = fopen('php://stdout', 'w');
+
+var_dump(stream_filter_append($fp, 'bzip2.i_dont_exist', STREAM_FILTER_WRITE));
+
+fclose($fp);
+
+?>
+--EXPECTF--
+Warning: stream_filter_append(): Unable to create or locate filter "bzip2.i_dont_exist" in %s on line %d
+bool(false)