fix: memory-safety bugs reachable from ordinary PHP

.github/workflows/ci.yml

@@ -25,7 +25,7 @@ jobs:
       - name: Install librrd
         run: |
           sudo apt-get update
-          sudo apt-get install -y librrd-dev pkg-config
+          sudo apt-get install -y librrd-dev pkg-config rrdtool
 
       - name: Setup PHP
         uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.37.2
@@ -40,6 +40,9 @@ jobs:
           ./configure --with-rrd
           make -j"$(nproc)"
 
+      - name: Generate test fixtures
+        run: make -C tests/data all
+
       - name: Test
         run: make test
         env:

.gitignore

@@ -29,3 +29,19 @@ modules
 run-tests.php
 tmp-php.ini
 tests/rrdtool-bin.inc
+*.dep
+configure.ac
+tests/*.diff
+tests/*.exp
+tests/*.log
+tests/*.out
+tests/*.php
+tests/*.sh
+tests/*.txt
+tests/*.png
+tests/*.rrd
+tests/data/*.png
+tests/data/*.rrd
+tests/data/*.txt
+tests/data/*.xml
+tests/data/Makefile

rrd.c

@@ -58,11 +58,12 @@ PHP_FUNCTION(rrd_fetch)
 	zval *zv_arr_options;
 	rrd_args *argv;
 	/* returned values if rrd_fetch doesn't fail */
-	time_t start, end;
-	unsigned long step,
-	ds_cnt; /* count of data sources */
-	char **ds_namv; /* list of data source names */
-	rrd_value_t *ds_data; /* all data from all sources */
+	time_t start = 0, end = 0;
+	unsigned long step = 0,
+	ds_cnt = 0; /* count of data sources */
+	unsigned ds_counter;
+	char **ds_namv = NULL; /* list of data source names */
+	rrd_value_t *ds_data = NULL; /* all data from all sources */
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "pa", &filename,
 		&filename_length, &zv_arr_options) == FAILURE) {
@@ -95,11 +96,11 @@ PHP_FUNCTION(rrd_fetch)
 	/* add "ds_namv" and "data" array into return values if there is any
 	 * result data
 	 */
-	if (!ds_data || !ds_namv || !ds_cnt) {
+	if (!ds_data || !ds_namv || !ds_cnt || !step) {
 		add_assoc_null(return_value, "data");
 	} else {
 		rrd_value_t *datap = ds_data;
-		unsigned timestamp, ds_counter;
+		time_t timestamp;
 		/* final array for all data from all data sources */
 		zval zv_data_array;
 
@@ -124,20 +125,26 @@ PHP_FUNCTION(rrd_fetch)
 				/* pointer for one data source retrieved data */
 				zval *ds_data_array;
 				/* value for key (timestamp) in data array */
-				char str_timestamp[11];
-				ZEND_LTOA((zend_ulong)timestamp, str_timestamp, sizeof(str_timestamp));
+				/* time_t is wider than zend_long on 32-bit builds, so
+				   this cannot go through ZEND_LONG_FMT */
+				char str_timestamp[24];
+				snprintf(str_timestamp, sizeof(str_timestamp),
+					"%" PRId64, (int64_t)timestamp);
 
-				/* gets pointer for data source result array */
+					/* gets pointer for data source result array */
 				ds_data_array = zend_hash_get_current_data(Z_ARRVAL(zv_data_array));
+				if (!ds_data_array) break;
 
 				add_assoc_double(ds_data_array, str_timestamp, *(datap++));
 				zend_hash_move_forward(Z_ARRVAL(zv_data_array));
 			}
 		}
 		add_assoc_zval(return_value, "data", &zv_data_array);
+	}
 
-		/* free data from rrd_fetch */
-		free(ds_data);
+	/* free data from rrd_fetch */
+	free(ds_data);
+	if (ds_namv) {
 		for (ds_counter = 0; ds_counter < ds_cnt; ds_counter++) {
 			free(ds_namv[ds_counter]);
 		}
@@ -399,10 +406,10 @@ PHP_FUNCTION(rrd_xport)
 	rrd_args *argv;
 	/* return values from rrd_xport */
 	int xxsize;
-	time_t start, end, time_index;
-	unsigned long step, outvar_count;
-	char **legend_v;
-	rrd_value_t *data, *data_ptr;
+	time_t start = 0, end = 0, time_index;
+	unsigned long step = 0, outvar_count = 0;
+	char **legend_v = NULL;
+	rrd_value_t *data = NULL, *data_ptr;
 	zval zv_data;
 	zend_ulong outvar_index;
 
@@ -436,8 +443,13 @@ PHP_FUNCTION(rrd_xport)
 	add_assoc_long(return_value, "step", step);
 
 	/* no data available */
-	if (!data) {
+	if (!data || !legend_v || !step) {
 		add_assoc_null(return_value, "data");
+		for (outvar_index = 0; legend_v && outvar_index < outvar_count; outvar_index++) {
+			free(legend_v[outvar_index]);
+		}
+		free(legend_v);
+		free(data);
 		return;
 	}
 
@@ -463,8 +475,9 @@ PHP_FUNCTION(rrd_xport)
 		data_ptr = data + outvar_index;
 		for (time_index = start + step; time_index <= end; time_index += step) {
 			/* value for key (timestamp) in data array */
-			char str_timestamp[11];
-			ZEND_LTOA((zend_ulong)time_index, str_timestamp, sizeof(str_timestamp));
+			char str_timestamp[24];
+			snprintf(str_timestamp, sizeof(str_timestamp),
+				"%" PRId64, (int64_t)time_index);
 
 			add_assoc_double(&time_data, str_timestamp, *data_ptr);
 			data_ptr += outvar_count;
@@ -664,13 +677,21 @@ rrd_args *rrd_args_init_by_phparray(const char *command_name, const char *filena
 	zend_hash_internal_pointer_reset(Z_ARRVAL_P(options));
 	for (i=0; i < option_count; i++) {
 		zval *item;
+		zend_string *item_str;
 		smart_string option = {0}; /* one argument option */
 
 		/* force using strings as array items */
 		item = zend_hash_get_current_data(Z_ARRVAL_P(options));
-		if (Z_TYPE_P(item) != IS_STRING) convert_to_string(item);
-		smart_string_appendl(&option, Z_STRVAL_P(item), Z_STRLEN_P(item));
+		item_str = zval_try_get_string(item);
+		if (!item_str) {
+			smart_string_free(&option);
+			result->count = args_counter;
+			rrd_args_free(result);
+			return NULL;
+		}
+		smart_string_appendl(&option, ZSTR_VAL(item_str), ZSTR_LEN(item_str));
 		smart_string_0(&option);
+		zend_string_release(item_str);
 
 		result->args[args_counter++] = estrdup(option.c);
 		smart_string_free(&option);

rrd_create.c

@@ -104,7 +104,7 @@ PHP_METHOD(RRDCreator, __construct)
 	rrd_create_object *intern_obj;
 	char *path; size_t path_length;
 	/* better to set defaults for optional parameters */
-	zend_string *start_time;
+	zend_string *start_time = NULL;
 	long step = 0;
 	int argc = ZEND_NUM_ARGS();
 
@@ -131,8 +131,27 @@ PHP_METHOD(RRDCreator, __construct)
 	}
 
 	intern_obj = php_rrd_create_fetch_object(Z_OBJ_P(getThis()));
-	intern_obj->file_path = estrdup(path);
-	if (start_time) intern_obj->start_time = estrdup(ZSTR_VAL(start_time));
+	if (intern_obj->file_path) efree(intern_obj->file_path);
+	intern_obj->file_path = NULL;
+	if (intern_obj->start_time) efree(intern_obj->start_time);
+	intern_obj->start_time = NULL;
+
+	/* a second __construct otherwise inherits the previous step and sources */
+	if (!Z_ISUNDEF(intern_obj->zv_step)) {
+		zval_dtor(&intern_obj->zv_step);
+		ZVAL_UNDEF(&intern_obj->zv_step);
+	}
+	if (!Z_ISUNDEF(intern_obj->zv_arr_data_sources)) {
+		zval_dtor(&intern_obj->zv_arr_data_sources);
+		ZVAL_UNDEF(&intern_obj->zv_arr_data_sources);
+	}
+	if (!Z_ISUNDEF(intern_obj->zv_arr_archives)) {
+		zval_dtor(&intern_obj->zv_arr_archives);
+		ZVAL_UNDEF(&intern_obj->zv_arr_archives);
+	}
+
+	intern_obj->file_path = estrndup(path, path_length);
+	if (start_time) intern_obj->start_time = estrndup(ZSTR_VAL(start_time), ZSTR_LEN(start_time));
 	if (step) {
 		ZVAL_LONG(&intern_obj->zv_step, step);
 	}
@@ -218,6 +237,15 @@ PHP_METHOD(RRDCreator, save)
 	zval zv_create_argv;
 	rrd_args *create_argv;
 
+	if (!intern_obj->file_path) {
+		zend_throw_exception(NULL, "the object was not constructed", 0);
+		return;
+	}
+
+	if (php_check_open_basedir(intern_obj->file_path)) {
+		RETURN_FALSE;
+	}
+
 	array_init(&zv_create_argv);
 
 	if (intern_obj->start_time) {
@@ -249,8 +277,12 @@ PHP_METHOD(RRDCreator, save)
 	}
 
 	/* add array of archive and data source strings into argument list */
-	php_array_merge(Z_ARRVAL(zv_create_argv), Z_ARRVAL(intern_obj->zv_arr_data_sources));
-	php_array_merge(Z_ARRVAL(zv_create_argv), Z_ARRVAL(intern_obj->zv_arr_archives));
+	if (!Z_ISUNDEF(intern_obj->zv_arr_data_sources)) {
+		php_array_merge(Z_ARRVAL(zv_create_argv), Z_ARRVAL(intern_obj->zv_arr_data_sources));
+	}
+	if (!Z_ISUNDEF(intern_obj->zv_arr_archives)) {
+		php_array_merge(Z_ARRVAL(zv_create_argv), Z_ARRVAL(intern_obj->zv_arr_archives));
+	}
 
 	create_argv = rrd_args_init_by_phparray("create", intern_obj->file_path, &zv_create_argv);
 	if (!create_argv) {

rrd_graph.c

@@ -100,7 +100,9 @@ PHP_METHOD(RRDGraph, __construct)
 	}
 
 	intern_obj = php_rrd_graph_fetch_object(Z_OBJ_P(getThis()));
-	intern_obj->file_path = estrdup(path);
+	if (intern_obj->file_path) efree(intern_obj->file_path);
+	intern_obj->file_path = NULL;
+	intern_obj->file_path = estrndup(path, path_length);
 }
 /* }}} */
 
@@ -131,7 +133,8 @@ PHP_METHOD(RRDGraph, setOptions)
 /* {{{
  creates arguments for rrd_graph call for RRDGraph instance options
 */
-static rrd_args *rrd_graph_obj_create_argv(const char *command_name, const rrd_graph_object *obj)
+static rrd_args *rrd_graph_obj_create_argv(const char *command_name,
+	const rrd_graph_object *obj, const char *file_path)
 {
 	/* iterated item and keys*/
 	zval *zv_option_val;
@@ -140,12 +143,18 @@ static rrd_args *rrd_graph_obj_create_argv(const char *command_name, const rrd_g
 	/* arguments for rrd_graph call as php array - temporary storage */
 	zval zv_argv;
 	rrd_args *result;
+	/* converting a value runs __toString, and setOptions() from there would
+	 * free the array this loop is walking
+	 */
+	HashTable *ht = Z_ARRVAL(obj->zv_arr_options);
 
+	GC_ADDREF(ht);
 	array_init(&zv_argv);
 
-	ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL(obj->zv_arr_options), num_key, zs_key, zv_option_val) {
+	ZEND_HASH_FOREACH_KEY_VAL(ht, num_key, zs_key, zv_option_val) {
 		(void)num_key; /* to avoid -Wunused-but-set-variable */
 		smart_string option = {0}; /* one argument option */
+		zend_string *option_str;
 
 		/* option with string key means long option, hence they are used as
 		 * "key=value" e.g. "--start=920804400"
@@ -156,19 +165,26 @@ static rrd_args *rrd_graph_obj_create_argv(const char *command_name, const rrd_g
 		}
 
 		/* use always string for option value */
-		if (Z_TYPE_P(zv_option_val) != IS_STRING) {
-			convert_to_string(zv_option_val);
+		option_str = zval_try_get_string(zv_option_val);
+		if (!option_str) {
+			smart_string_free(&option);
+			zval_dtor(&zv_argv);
+			zend_array_release(ht);
+			return NULL;
 		}
 
-		smart_string_appendl(&option, Z_STRVAL_P(zv_option_val), Z_STRLEN_P(zv_option_val));
+		smart_string_appendl(&option, ZSTR_VAL(option_str), ZSTR_LEN(option_str));
 		smart_string_0(&option);
+		zend_string_release(option_str);
 
 		add_next_index_string(&zv_argv, option.c);
 
 		smart_string_free(&option);
 	} ZEND_HASH_FOREACH_END();
 
-	result = rrd_args_init_by_phparray(command_name, obj->file_path, &zv_argv);
+	zend_array_release(ht);
+
+	result = rrd_args_init_by_phparray(command_name, file_path, &zv_argv);
 	zval_dtor(&zv_argv);
 
 	return result;
@@ -189,6 +205,12 @@ PHP_METHOD(RRDGraph, save)
 
 	/* arguments for rrd_graph call */
 	rrd_args *graph_argv;
+	char *checked_path;
+
+	if (!intern_obj->file_path) {
+		zend_throw_exception(NULL, "the object was not constructed", 0);
+		return;
+	}
 
 	if (Z_TYPE(intern_obj->zv_arr_options) != IS_ARRAY) {
 		zend_throw_exception(NULL, "options aren't correctly set", 0);
@@ -199,7 +221,9 @@ PHP_METHOD(RRDGraph, save)
 		RETURN_FALSE;
 	}
 
-	graph_argv = rrd_graph_obj_create_argv("graph", intern_obj);
+	checked_path = estrdup(intern_obj->file_path);
+	graph_argv = rrd_graph_obj_create_argv("graph", intern_obj, checked_path);
+	efree(checked_path);
 	if (!graph_argv) {
 		zend_error(E_WARNING, "cannot allocate arguments options");
 		RETURN_FALSE;
@@ -260,13 +284,25 @@ PHP_METHOD(RRDGraph, saveVerbose)
 
 	/* arguments for rrd_graph call */
 	rrd_args *graph_argv;
+	char *checked_path;
+
+	if (!intern_obj->file_path) {
+		zend_throw_exception(NULL, "the object was not constructed", 0);
+		return;
+	}
 
 	if (Z_TYPE(intern_obj->zv_arr_options) != IS_ARRAY) {
-		zend_throw_exception(NULL, "options aren't correctly set", 0); 
+		zend_throw_exception(NULL, "options aren't correctly set", 0);
 		return;
 	}
 
-	graph_argv = rrd_graph_obj_create_argv("graphv", intern_obj);
+	if (php_check_open_basedir(intern_obj->file_path)) {
+		RETURN_FALSE;
+	}
+
+	checked_path = estrdup(intern_obj->file_path);
+	graph_argv = rrd_graph_obj_create_argv("graphv", intern_obj, checked_path);
+	efree(checked_path);
 	if (!graph_argv) {
 		zend_error(E_WARNING, "cannot allocate arguments options");
 		RETURN_FALSE;