ext/ldap: Various refactorings to php_ldap_do_search()

ext/ldap/ldap.c

@@ -1402,19 +1402,20 @@ static void php_set_opts(LDAP *ldap, int sizelimit, int timelimit, int deref, in
 /* {{{ php_ldap_do_search */
 static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 {
-	zval *link, *attrs = NULL, *attr, *serverctrls = NULL;
-	zend_string *base_dn_str, *filter_str;
-	HashTable *base_dn_ht, *filter_ht;
+	zval *link, *attrs = NULL, *serverctrls = NULL;
+	HashTable *base_dn_ht = NULL;
+	zend_string *base_dn_str = NULL;
+	HashTable *filter_ht = NULL;
+	zend_string *filter_str = NULL;
 	zend_long attrsonly, sizelimit, timelimit, deref;
-	zend_string *ldap_filter = NULL, *ldap_base_dn = NULL;
 	char **ldap_attrs = NULL;
 	ldap_linkdata *ld = NULL;
 	ldap_resultdata *result;
 	LDAPMessage *ldap_res = NULL;
 	LDAPControl **lserverctrls = NULL;
 	int ldap_attrsonly = 0, ldap_sizelimit = -1, ldap_timelimit = -1, ldap_deref = -1;
 	int old_ldap_sizelimit = -1, old_ldap_timelimit = -1, old_ldap_deref = -1;
-	int num_attribs = 0, ret = 1, i, ldap_errno, argcount = ZEND_NUM_ARGS();
+	int ret = 1, ldap_errno, argcount = ZEND_NUM_ARGS();
 
 	ZEND_PARSE_PARAMETERS_START(3, 9)
 		Z_PARAM_ZVAL(link)
@@ -1444,142 +1445,190 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 		case 5:
 			ldap_attrsonly = attrsonly;
 			ZEND_FALLTHROUGH;
-		case 4:
-			num_attribs = zend_hash_num_elements(Z_ARRVAL_P(attrs));
-			ldap_attrs = safe_emalloc((num_attribs+1), sizeof(char *), 0);
+		default:
+			break;
+	}
 
-			for (i = 0; i<num_attribs; i++) {
-				if ((attr = zend_hash_index_find(Z_ARRVAL_P(attrs), i)) == NULL) {
-					php_error_docref(NULL, E_WARNING, "Array initialization wrong");
-					ret = 0;
-					goto cleanup;
-				}
+	if (attrs) {
+		const HashTable *attributes = Z_ARRVAL_P(attrs);
+		uint32_t num_attribs = zend_hash_num_elements(attributes);
 
-				convert_to_string(attr);
-				if (EG(exception)) {
-					ret = 0;
-					goto cleanup;
-				}
-				ldap_attrs[i] = Z_STRVAL_P(attr);
+		if (num_attribs == 0) {
+			/* 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");
+			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) {
+			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));
+				ret = 0;
+				goto cleanup;
 			}
-			ldap_attrs[num_attribs] = NULL;
-			ZEND_FALLTHROUGH;
-		default:
-			break;
+			zend_string *attribute = Z_STR_P(attribute_zv);
+			if (zend_str_has_nul_byte(attribute)) {
+				zend_argument_value_error(4, "must not contain strings with any null bytes");
+				ret = 0;
+				goto cleanup;
+			}
+			ldap_attrs[attribute_index] = ZSTR_VAL(attribute);
+		} ZEND_HASH_FOREACH_END();
+		ldap_attrs[num_attribs] = NULL;
 	}
+process:
 
 	/* parallel search? */
 	if (Z_TYPE_P(link) == IS_ARRAY) {
-		int i, nlinks, nbases, nfilters, *rcs;
-		ldap_linkdata **lds;
-		zval *entry, object;
-
-		nlinks = zend_hash_num_elements(Z_ARRVAL_P(link));
-		if (nlinks == 0) {
+		const zend_string *ldap_base_dn = NULL;
+		const zend_string *ldap_filter = NULL;
+		uint32_t num_links = zend_hash_num_elements(Z_ARRVAL_P(link));
+		if (num_links == 0) {
 			zend_argument_must_not_be_empty_error(1);
 			ret = 0;
 			goto cleanup;
 		}
+		if (!zend_array_is_list(Z_ARRVAL_P(link))) {
+			zend_argument_value_error(1, "must be a list");
+			ret = 0;
+			goto cleanup;
+		}
 
+		uint32_t num_base_dns = 0; /* If 0 this means we are working with a unique base dn */
 		if (base_dn_ht) {
-			nbases = zend_hash_num_elements(base_dn_ht);
-			if (nbases != nlinks) {
-				zend_argument_value_error(2, "must have the same number of elements as the links array");
+			if (!zend_array_is_list(base_dn_ht)) {
+				zend_argument_value_error(2, "must be a list");
+				ret = 0;
+				goto cleanup;
+			}
+			num_base_dns = zend_hash_num_elements(base_dn_ht);
+			if (num_base_dns != num_links) {
+				zend_argument_value_error(2, "must be the same size as argument #1");
 				ret = 0;
 				goto cleanup;
 			}
-			zend_hash_internal_pointer_reset(base_dn_ht);
 		} else {
-			nbases = 0; /* this means string, not array */
-			ldap_base_dn = zend_string_copy(base_dn_str);
-			if (EG(exception)) {
+			if (zend_str_has_nul_byte(base_dn_str)) {
+				zend_argument_value_error(2, "must not contain null bytes");
 				ret = 0;
 				goto cleanup;
 			}
-			// TODO check filter does not have any nul bytes
+			ldap_base_dn = base_dn_str;
 		}
 
+		uint32_t num_filters = 0; /* If 0 this means we are working with a unique base dn */
 		if (filter_ht) {
-			nfilters = zend_hash_num_elements(filter_ht);
-			if (nfilters != nlinks) {
-				zend_argument_value_error(3, "must have the same number of elements as the links array");
+			if (!zend_array_is_list(filter_ht)) {
+				zend_argument_value_error(3, "must be a list");
+				ret = 0;
+				goto cleanup;
+			}
+			num_filters = zend_hash_num_elements(filter_ht);
+			if (num_filters != num_links) {
+				zend_argument_value_error(3, "must be the same size as argument #1");
 				ret = 0;
 				goto cleanup;
 			}
-			zend_hash_internal_pointer_reset(filter_ht);
 		} else {
-			nfilters = 0; /* this means string, not array */
-			ldap_filter = zend_string_copy(filter_str);
-			// TODO check filter does not have any nul bytes
+			if (zend_str_has_nul_byte(filter_str)) {
+				zend_argument_value_error(3, "must not contain null bytes");
+				ret = 0;
+				goto cleanup;
+			}
+			ldap_filter = filter_str;
 		}
 
-		lds = safe_emalloc(nlinks, sizeof(ldap_linkdata), 0);
-		rcs = safe_emalloc(nlinks, sizeof(*rcs), 0);
-
-		zend_hash_internal_pointer_reset(Z_ARRVAL_P(link));
-		for (i=0; i<nlinks; i++) {
-			entry = zend_hash_get_current_data(Z_ARRVAL_P(link));
-
-			if (Z_TYPE_P(entry) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(entry), ldap_link_ce)) {
-				zend_argument_value_error(1, "must only contain objects of type LDAP");
+		int *rcs;
+		ldap_linkdata **lds;
+		lds = safe_emalloc(num_links, sizeof(ldap_linkdata), 0);
+		rcs = safe_emalloc(num_links, sizeof(*rcs), 0);
+
+		zend_ulong ldap_link_index = 0;
+		zval *link_zv = NULL;
+		ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARRVAL_P(link), ldap_link_index, link_zv) {
+			ZVAL_DEREF(link_zv);
+			if (Z_TYPE_P(link_zv) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(link_zv), ldap_link_ce)) {
+				zend_argument_value_error(1, "must be a list of LDAP\\Connection");
 				ret = 0;
 				goto cleanup_parallel;
 			}
 
-			ld = Z_LDAP_LINK_P(entry);
-			if (!ld->link) {
+			ldap_linkdata *current_ld = Z_LDAP_LINK_P(link_zv);
+			if (!current_ld->link) {
 				zend_throw_error(NULL, "LDAP connection has already been closed");
 				ret = 0;
 				goto cleanup_parallel;
 			}
 
-			if (nbases != 0) { /* base_dn an array? */
-				entry = zend_hash_get_current_data(base_dn_ht);
-				zend_hash_move_forward(base_dn_ht);
-				ldap_base_dn = zval_get_string(entry);
-				if (EG(exception)) {
+			if (num_base_dns != 0) { /* base_dn an array? */
+				zval *base_dn_zv = zend_hash_index_find(base_dn_ht, ldap_link_index);
+				ZEND_ASSERT(base_dn_zv);
+				ZVAL_DEREF(base_dn_zv);
+				if (Z_TYPE_P(base_dn_zv) != IS_STRING) {
+					zend_argument_type_error(2, "must be a list of strings, %s given", zend_zval_value_name(base_dn_zv));
+					ret = 0;
+					goto cleanup_parallel;
+				}
+				ldap_base_dn = Z_STR_P(base_dn_zv);
+				if (zend_str_has_nul_byte(ldap_base_dn)) {
+					zend_argument_value_error(2, "must not contain null bytes");
 					ret = 0;
 					goto cleanup_parallel;
 				}
-				// TODO check dn does not have any nul bytes
 			}
-			if (nfilters != 0) { /* filter an array? */
-				entry = zend_hash_get_current_data(filter_ht);
-				zend_hash_move_forward(filter_ht);
-				ldap_filter = zval_get_string(entry);
-				if (EG(exception)) {
+			if (num_filters != 0) { /* filter an array? */
+				zval *filter_zv = zend_hash_index_find(filter_ht, ldap_link_index);
+				ZEND_ASSERT(filter_zv);
+				ZVAL_DEREF(filter_zv);
+				if (Z_TYPE_P(filter_zv) != IS_STRING) {
+					zend_argument_type_error(3, "must be a list of strings, %s given", zend_zval_value_name(filter_zv));
+					ret = 0;
+					goto cleanup_parallel;
+				}
+				ldap_filter = Z_STR_P(filter_zv);
+				if (zend_str_has_nul_byte(ldap_filter)) {
+					zend_argument_value_error(3, "must not contain null bytes");
 					ret = 0;
 					goto cleanup_parallel;
 				}
-				// TODO check filter does not have any nul bytes
 			}
 
 			if (serverctrls) {
 				/* We have to parse controls again for each link as they use it */
 				_php_ldap_controls_free(&lserverctrls);
-				lserverctrls = _php_ldap_controls_from_array(ld->link, serverctrls, 9);
+				lserverctrls = _php_ldap_controls_from_array(current_ld->link, serverctrls, 9);
 				if (lserverctrls == NULL) {
-					rcs[i] = -1;
+					rcs[ldap_link_index] = -1;
+					// TODO Throw an exception/cleanup?
 					continue;
 				}
 			}
 
-			php_set_opts(ld->link, ldap_sizelimit, ldap_timelimit, ldap_deref, &old_ldap_sizelimit, &old_ldap_timelimit, &old_ldap_deref);
+			php_set_opts(current_ld->link, ldap_sizelimit, ldap_timelimit, ldap_deref, &old_ldap_sizelimit, &old_ldap_timelimit, &old_ldap_deref);
 
 			/* Run the actual search */
-			ldap_search_ext(ld->link, ZSTR_VAL(ldap_base_dn), scope, ZSTR_VAL(ldap_filter), ldap_attrs, ldap_attrsonly, lserverctrls, NULL, NULL, ldap_sizelimit, &rcs[i]);
-			lds[i] = ld;
-			zend_hash_move_forward(Z_ARRVAL_P(link));
-		}
+			ldap_search_ext(current_ld->link, ZSTR_VAL(ldap_base_dn), scope, ZSTR_VAL(ldap_filter), ldap_attrs, ldap_attrsonly, lserverctrls, NULL, NULL, ldap_sizelimit, &rcs[ldap_link_index]);
+			lds[ldap_link_index] = current_ld;
+
+			// TODO Reset the options of the link?
+		} ZEND_HASH_FOREACH_END();
 
 		array_init(return_value);
 
 		/* Collect results from the searches */
-		for (i=0; i<nlinks; i++) {
+		for (uint32_t i = 0; i < num_links; i++) {
 			if (rcs[i] != -1) {
 				rcs[i] = ldap_result(lds[i]->link, LDAP_RES_ANY, 1 /* LDAP_MSG_ALL */, NULL, &ldap_res);
 			}
 			if (rcs[i] != -1) {
+				zval object;
 				object_init_ex(&object, ldap_result_ce);
 				result = Z_LDAP_RESULT_P(&object);
 				result->result = ldap_res;
@@ -1601,18 +1650,16 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 		}
 
 		if (!base_dn_str) {
-			zend_argument_type_error(2, "must be of type string when argument #1 ($ldap) is an LDAP instance");
+			zend_argument_type_error(2, "must be of type string when argument #1 ($ldap) is an LDAP\\Connection instance");
 			ret = 0;
 			goto cleanup;
 		}
-		ldap_base_dn = zend_string_copy(base_dn_str);
 
 		if (!filter_str) {
-			zend_argument_type_error(3, "must be of type string when argument #1 ($ldap) is an LDAP instance");
+			zend_argument_type_error(3, "must be of type string when argument #1 ($ldap) is an LDAP\\Connection instance");
 			ret = 0;
 			goto cleanup;
 		}
-		ldap_filter = zend_string_copy(filter_str);
 
 		if (serverctrls) {
 			lserverctrls = _php_ldap_controls_from_array(ld->link, serverctrls, 9);
@@ -1625,7 +1672,7 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 		php_set_opts(ld->link, ldap_sizelimit, ldap_timelimit, ldap_deref, &old_ldap_sizelimit, &old_ldap_timelimit, &old_ldap_deref);
 
 		/* Run the actual search */
-		ldap_errno = ldap_search_ext_s(ld->link, ZSTR_VAL(ldap_base_dn), scope, ZSTR_VAL(ldap_filter), ldap_attrs, ldap_attrsonly, lserverctrls, NULL, NULL, ldap_sizelimit, &ldap_res);
+		ldap_errno = ldap_search_ext_s(ld->link, ZSTR_VAL(base_dn_str), scope, ZSTR_VAL(filter_str), ldap_attrs, ldap_attrsonly, lserverctrls, NULL, NULL, ldap_sizelimit, &ldap_res);
 
 		if (ldap_errno != LDAP_SUCCESS
 			&& ldap_errno != LDAP_SIZELIMIT_EXCEEDED
@@ -1657,20 +1704,14 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope)
 			result->result = ldap_res;
 		}
 	} else {
-		zend_argument_type_error(1, "must be of type LDAP|array, %s given", zend_zval_value_name(link));
+		zend_argument_type_error(1, "must be of type LDAP\\Connection|array, %s given", zend_zval_value_name(link));
 	}
 
 cleanup:
 	if (ld) {
 		/* Restoring previous options */
 		php_set_opts(ld->link, old_ldap_sizelimit, old_ldap_timelimit, old_ldap_deref, &ldap_sizelimit, &ldap_timelimit, &ldap_deref);
 	}
-	if (ldap_filter) {
-		zend_string_release(ldap_filter);
-	}
-	if (ldap_base_dn) {
-		zend_string_release(ldap_base_dn);
-	}
 	if (ldap_attrs != NULL) {
 		efree(ldap_attrs);
 	}

ext/ldap/tests/gh16101.phpt

@@ -0,0 +1,25 @@
+--TEST--
+Bug GH-16101: Segfault in ldap_list(), ldap_read(), and ldap_search() when LDAPs array is not a list
+--EXTENSIONS--
+ldap
+--FILE--
+<?php
+
+/* We are assuming 3333 is not connectable */
+$ldap = ldap_connect('ldap://127.0.0.1:3333');
+$valid_dn = "cn=userA,something";
+$valid_filter = "";
+
+$ldaps_dict = [
+    "hello"  => $ldap,
+    "world"  => $ldap,
+];
+try {
+    var_dump(ldap_list($ldaps_dict, $valid_dn, $valid_filter));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+
+?>
+--EXPECT--
+ValueError: ldap_list(): Argument #1 ($ldap) must be a list