Fix GH-17280: ldap_search() fails when $attributes array has holes (#17284)

We relax the constraint that the array must be a list. What really
matters is that it only has numeric keys. As shown in the example code,
it's really easy to accidentally create a non-list array, so it makes
sense to relax the constraint.
There are 3 cases left where the array is checked to be a list,
in php_ldap_do_search, but I believe this makes sense to keep because
the indices of those arrays have a meaning because they should match
between different arrays. In that case it will prevent programmer
errors.

Author: nielsdos

ext/ldap/ldap.c

@@ -234,6 +234,22 @@ static void ldap_result_entry_free_obj(zend_object *obj)
 	} \
 }
 
+static bool php_ldap_is_numerically_indexed_array(const zend_array *arr)
+{
+	if (zend_hash_num_elements(arr) == 0 || HT_IS_PACKED(arr)) {
+		return true;
+	}
+
+	zend_string *str_key;
+	ZEND_HASH_MAP_FOREACH_STR_KEY(arr, str_key) {
+		if (str_key) {
+			return false;
+		}
+	} ZEND_HASH_FOREACH_END();
+
+	return true;
+}
+
 /* An LDAP value must be a string, however it defines a format for integer and
  * booleans, thus we parse zvals to the corresponding string if possible
  * See RFC 4517: https://datatracker.ietf.org/doc/html/rfc4517 */
@@ -1481,16 +1497,16 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 			/* We don't allocate ldap_attrs for an empty array */
 			goto process;
 		}
-		if (!zend_array_is_list(attributes)) {
-			zend_argument_value_error(4, "must be a list");
+		if (!php_ldap_is_numerically_indexed_array(attributes)) {
+			zend_argument_value_error(4, "must be an array with numeric keys");
 			RETURN_THROWS();
 		}
 		/* Allocate +1 as we need an extra entry to NULL terminate the list */
 		ldap_attrs = safe_emalloc(num_attribs+1, sizeof(char *), 0);
 
 		zend_ulong attribute_index = 0;
 		zval *attribute_zv = NULL;
-		ZEND_HASH_FOREACH_NUM_KEY_VAL(attributes, attribute_index, attribute_zv) {
+		ZEND_HASH_FOREACH_VAL(attributes, attribute_zv) {
 			ZVAL_DEREF(attribute_zv);
 			if (Z_TYPE_P(attribute_zv) != IS_STRING) {
 				zend_argument_type_error(4, "must be a list of strings, %s given", zend_zval_value_name(attribute_zv));
@@ -1503,7 +1519,7 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 				ret = 0;
 				goto cleanup;
 			}
-			ldap_attrs[attribute_index] = ZSTR_VAL(attribute);
+			ldap_attrs[attribute_index++] = ZSTR_VAL(attribute);
 		} ZEND_HASH_FOREACH_END();
 		ldap_attrs[num_attribs] = NULL;
 	}
@@ -2302,8 +2318,8 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext)
 				RETVAL_FALSE;
 				goto cleanup;
 			}
-			if (!zend_array_is_list(Z_ARRVAL_P(attribute_values))) {
-				zend_argument_value_error(3, "must be a list of attribute values");
+			if (!php_ldap_is_numerically_indexed_array(Z_ARRVAL_P(attribute_values))) {
+				zend_argument_value_error(3, "must be an array of attribute values with numeric keys");
 				RETVAL_FALSE;
 				goto cleanup;
 			}
@@ -2314,7 +2330,7 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext)
 
 			zend_ulong attribute_value_index = 0;
 			zval *attribute_value = NULL;
-			ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARRVAL_P(attribute_values), attribute_value_index, attribute_value) {
+			ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(attribute_values), attribute_value) {
 				zend_string *value = php_ldap_try_get_ldap_value_from_zval(attribute_value);
 				if (UNEXPECTED(value == NULL)) {
 					RETVAL_FALSE;
@@ -2324,6 +2340,7 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext)
 				/* The string will be free by php_ldap_zend_string_release_from_char_pointer() during cleanup */
 				ldap_mods[attribute_index]->mod_bvalues[attribute_value_index]->bv_val = ZSTR_VAL(value);
 				ldap_mods[attribute_index]->mod_bvalues[attribute_value_index]->bv_len = ZSTR_LEN(value);
+				attribute_value_index++;
 			} ZEND_HASH_FOREACH_END();
 			ldap_mods[attribute_index]->mod_bvalues[num_values] = NULL;
 		}
@@ -2594,8 +2611,8 @@ PHP_FUNCTION(ldap_modify_batch)
 		zend_argument_must_not_be_empty_error(3);
 		RETURN_THROWS();
 	}
-	if (!zend_array_is_list(modifications)) {
-		zend_argument_value_error(3, "must be a list");
+	if (!php_ldap_is_numerically_indexed_array(modifications)) {
+		zend_argument_value_error(3, "must be an array with numeric keys");
 		RETURN_THROWS();
 	}
 
@@ -2689,8 +2706,8 @@ PHP_FUNCTION(ldap_modify_batch)
 			zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must not be empty");
 			RETURN_THROWS();
 		}
-		if (!zend_array_is_list(modification_values)) {
-			zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must be a list");
+		if (!php_ldap_is_numerically_indexed_array(modification_values)) {
+			zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must be an array with numeric keys");
 			RETURN_THROWS();
 		}
 	} ZEND_HASH_FOREACH_END();
@@ -2752,7 +2769,7 @@ PHP_FUNCTION(ldap_modify_batch)
 			/* for each value */
 			zend_ulong value_index = 0;
 			zval *modification_value_zv = NULL;
-			ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARRVAL_P(modification_values), value_index, modification_value_zv) {
+			ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(modification_values), modification_value_zv) {
 				zend_string *modval = zval_get_string(modification_value_zv);
 				if (EG(exception)) {
 					RETVAL_FALSE;
@@ -2768,6 +2785,7 @@ PHP_FUNCTION(ldap_modify_batch)
 				ldap_mods[modification_index]->mod_bvalues[value_index]->bv_len = ZSTR_LEN(modval);
 				ldap_mods[modification_index]->mod_bvalues[value_index]->bv_val = estrndup(ZSTR_VAL(modval), ZSTR_LEN(modval));
 				zend_string_release(modval);
+				value_index++;
 			} ZEND_HASH_FOREACH_END();
 
 			/* NULL-terminate values */

ext/ldap/tests/gh17280.phpt

@@ -0,0 +1,18 @@
+--TEST--
+GH-17280 (ldap_search() fails when $attributes array has holes)
+--EXTENSIONS--
+ldap
+--FILE--
+<?php
+
+/* We are assuming 3333 is not connectable */
+$ldap = ldap_connect('ldap://127.0.0.1:3333');
+
+// Creating an array with a hole in it
+$attr = array_unique(['cn', 'uid', 'uid', 'mail']);
+var_dump(ldap_search($ldap, 'ou=people,dc=example,dc=com', 'uid=admin', $attr));
+
+?>
+--EXPECTF--
+Warning: ldap_search(): Search: Can't contact LDAP server in %s on line %d
+bool(false)

ext/ldap/tests/ldap_add_modify_delete_programming_errors.phpt

@@ -138,6 +138,6 @@ ValueError: ldap_add(): Argument #3 ($entry) key must not be empty
 ValueError: ldap_add(): Argument #3 ($entry) key must not contain any null bytes
 Error: Object of class stdClass could not be converted to string
 ValueError: ldap_add(): Argument #3 ($entry) list of attribute values must not be empty
-ValueError: ldap_add(): Argument #3 ($entry) must be a list of attribute values
+ValueError: ldap_add(): Argument #3 ($entry) must be an array of attribute values with numeric keys
 TypeError: LDAP value must be of type string|int|bool, array given
 Error: Object of class stdClass could not be converted to string

ext/ldap/tests/ldap_list_read_search_programming_errors.phpt

@@ -80,7 +80,7 @@ try {
 TypeError: ldap_list(): Argument #1 ($ldap) must be of type LDAP\Connection|array, int given
 TypeError: ldap_list(): Argument #2 ($base) must be of type string when argument #1 ($ldap) is an LDAP\Connection instance
 TypeError: ldap_list(): Argument #3 ($filter) must be of type string when argument #1 ($ldap) is an LDAP\Connection instance
-ValueError: ldap_list(): Argument #4 ($attributes) must be a list
+ValueError: ldap_list(): Argument #4 ($attributes) must be an array with numeric keys
 TypeError: ldap_list(): Argument #4 ($attributes) must be a list of strings, int given
 ValueError: ldap_list(): Argument #4 ($attributes) must not contain strings with any null bytes
 ValueError: ldap_list(): Argument #4 ($attributes) must not contain strings with any null bytes

ext/ldap/tests/ldap_modify_batch_programming_error.phpt

@@ -216,19 +216,6 @@ try {
     echo $e::class, ': ', $e->getMessage(), PHP_EOL;
 }
 
-$modification_values_not_list2 = [
-    [
-        "attrib"  => "attrib1",
-        "modtype" => LDAP_MODIFY_BATCH_ADD,
-        "values"  => $not_list2,
-    ],
-];
-try {
-    var_dump(ldap_modify_batch($ldap, $valid_dn, $modification_values_not_list2));
-} catch (Throwable $e) {
-    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
-}
-
 $modification_missing_attrib_key = [
     [
         "modtype" => LDAP_MODIFY_BATCH_ADD,
@@ -257,8 +244,8 @@ try {
 --EXPECT--
 ValueError: ldap_modify_batch(): Argument #2 ($dn) must not contain any null bytes
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must not be empty
-ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be a list
-ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be a list
+ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be an array with numeric keys
+ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must only contain the keys "attrib", "modtype", and "values"
 TypeError: ldap_modify_batch(): Argument #3 ($modifications_info) must only contain arrays
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must not contain the "values" option when option "modtype" is LDAP_MODIFY_BATCH_REMOVE_ALL
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must only contain the keys "attrib", "modtype", and "values"
@@ -270,7 +257,6 @@ ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modificatio
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "values" option when the "modtype" option is not LDAP_MODIFY_BATCH_REMOVE_ALL
 TypeError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be of type array, string given
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must not be empty
-ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be a list
-ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be a list
+ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be an array with numeric keys
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "attrib" option
 ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "modtype" option

ext/ldap/tests/ldap_search_error.phpt

@@ -20,7 +20,7 @@ $result = ldap_search($link, $dn, $filter);
 var_dump($result);
 
 try {
-    $result = ldap_search($link, $dn, $filter, array(1 => 'top'));
+    $result = ldap_search($link, $dn, $filter, array('foo' => 'top'));
     var_dump($result);
 } catch (ValueError $exception) {
     echo $exception->getMessage() . "\n";
@@ -60,7 +60,7 @@ try {
 --EXPECTF--
 Warning: ldap_search(): Search: No such object in %s on line %d
 bool(false)
-ldap_search(): Argument #4 ($attributes) must be a list
+ldap_search(): Argument #4 ($attributes) must be an array with numeric keys
 ldap_search(): Argument #1 ($ldap) must not be empty
 ldap_search(): Argument #2 ($base) must be the same size as argument #1
 ldap_search(): Argument #3 ($filter) must be the same size as argument #1