bz2: refactor filter creation to do parameter validation

ext/bz2/bz2_filter.c

@@ -21,7 +21,7 @@
 
 /* {{{ data structure */
 
-enum strm_status {
+C23_ENUM(strm_status, uint8_t) {
 	PHP_BZ2_UNINITIALIZED,
 	PHP_BZ2_RUNNING,
 	PHP_BZ2_FINISHED
@@ -34,12 +34,11 @@ typedef struct _php_bz2_filter_data {
 	size_t inbuf_len;
 	size_t outbuf_len;
 
-	enum strm_status status;              /* Decompress option */
-	unsigned int small_footprint : 1;     /* Decompress option */
-	unsigned int expect_concatenated : 1; /* Decompress option */
-	unsigned int is_flushed : 1;          /* only for compression */
-
-	int persistent;
+	bool persistent;
+	bool expect_concatenated : 1; /* Decompress option */
+	bool small_footprint : 1;     /* Decompress option */
+	bool is_flushed : 1;          /* only for compression */
+	strm_status status;           /* Decompress option */
 
 	/* Configuration for reset - immutable */
 	int blockSize100k;  /* compress only */
@@ -376,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;
 
@@ -402,86 +387,137 @@ 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;
+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 (filterparams) {
-			zval *tmpzval = NULL;
+	if (filter_params) {
+		if (Z_TYPE_P(filter_params) == IS_TRUE || Z_TYPE_P(filter_params) == IS_FALSE) {
+			small_footprint = Z_TYPE_P(filter_params) == IS_TRUE;
+			goto allocate_decompress_filter;
+		}
+		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.decompress filter must be of type array|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);
+		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;
+		}
 
-				if ((tmpzval = zend_hash_str_find_ind(ht, "concatenated", sizeof("concatenated")-1))) {
-					data->expect_concatenated = zend_is_true(tmpzval);
-					tmpzval = NULL;
-				}
+		const zval *concatenated = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("concatenated"));
+		if (concatenated) {
+			expect_concatenated = zend_is_true(concatenated);
+		}
 
-				tmpzval = zend_hash_str_find_ind(ht, "small", sizeof("small")-1);
-			} else {
-				tmpzval = filterparams;
-			}
+		const zval *small = zend_hash_str_find_ind(filter_params_ht, ZEND_STRL("small"));
+		if (small) {
+			small_footprint = zend_is_true(small);
+		}
+	}
 
-			if (tmpzval) {
-				data->small_footprint = zend_is_true(tmpzval);
-			}
+allocate_decompress_filter:;
+	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;
+
+	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, %s given",
+				zend_zval_type_name(filter_params)
+			);
+			return NULL;
 		}
 
-		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;
-					}
-				}
+		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;
+		}
 
-				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;
-					}
-				}
+		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;
 			}
 		}
 
-		/* Save configuration for reset */
-		data->blockSize100k = blockSize100k;
-		data->workFactor = workFactor;
-
-		status = BZ2_bzCompressInit(&(data->strm), blockSize100k, 0, workFactor);
-		data->is_flushed = 1;
-		fops = &php_bz2_compress_ops;
-	} else {
-		status = BZ_DATA_ERROR;
+		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, 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|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.decompress filter param errors
+--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)