Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:28182 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 30044 invoked by uid 1010); 28 Feb 2007 20:33:14 -0000 Delivered-To: ezmlm-scan-internals@lists.php.net Delivered-To: ezmlm-internals@lists.php.net Received: (qmail 30028 invoked from network); 28 Feb 2007 20:33:14 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 28 Feb 2007 20:33:14 -0000 Authentication-Results: pb1.pair.com smtp.mail=cardoe@gentoo.org; spf=unknown; sender-id=unknown Authentication-Results: pb1.pair.com header.from=cardoe@gentoo.org; sender-id=unknown Received-SPF: unknown (pb1.pair.com: domain gentoo.org does not designate 64.111.100.16 as permitted sender) X-PHP-List-Original-Sender: cardoe@gentoo.org X-Host-Fingerprint: 64.111.100.16 webmail4.sd.dreamhost.com Linux 2.4/2.6 Received: from [64.111.100.16] ([64.111.100.16:34739] helo=webmail4.sd.dreamhost.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id F8/11-34076-807E5E54 for ; Wed, 28 Feb 2007 15:33:13 -0500 Received: from webmail.cardoe.com (localhost [127.0.0.1]) by webmail4.sd.dreamhost.com (Postfix) with ESMTP id 6E1AC302A0 for ; Wed, 28 Feb 2007 12:33:10 -0800 (PST) Received: from 216.155.111.10 (SquirrelMail authenticated user cardoe@cardoe.com) by webmail.cardoe.com with HTTP; Wed, 28 Feb 2007 15:33:10 -0500 (EST) Message-ID: <40059.216.155.111.10.1172694790.squirrel@webmail.cardoe.com> Date: Wed, 28 Feb 2007 15:33:10 -0500 (EST) To: internals@lists.php.net Reply-To: cardoe@gentoo.org User-Agent: SquirrelMail/1.4.9a MIME-Version: 1.0 Content-Type: multipart/mixed;boundary="----=_20070228153310_59904" Subject: LDAP functions implemented poorly From: cardoe@gentoo.org ("Doug Goldstein") ------=_20070228153310_59904 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8bit Referencing Bug #38819 & Bug #40671 http://bugs.php.net/bug.php?id=38819 http://bugs.php.net/bug.php?id=40671 Essentially I looked through the above mentioned bug, the bugs opened with OpenLDAP developers, and then reviewed ext/ldap/ldap.c and it appears the API calls made by PHP are not necessarily the safest ways to write the PHP wrapper functions. Based on tony2001@php.net's comment that the LDAP module is unmaintained I went ahead and made some changes. If you read OpenLDAP's API and comments by OpenLDAP Core Developers, available at: http://www.openldap.org/its/index.cgi/Build?id=4690;selectid=4690 http://www.openldap.org/software/man.cgi?query=ldap_get_values&sektion=3 &apropos=0&manpath=OpenLDAP+2.1-Release (Notice I went with OpenLDAP 2.1 docs to quell PHP's urge for backwards compatibility) The functions char **ldap_get_values(ld, entry, attr) and struct berval **ldap_get_values_len(ld, entry, attr) are essentially inter-changeable. The big difference being that the berval struct provides you with a char * and the size_t of the data. Rather then just a char * that you then have to strlen() which will result in problems if the returned data is not NULL terminated data. PHP's internal functions make the mistake of assuming all data will be string data (NULL terminated char *) data, which is the cause of the crash in bug #38819. The patch attached removes all of those assumptions and uses ldap_get_values_len() and uses the length provided back by the structure to feed add_index_stringl() instead of using add_index_string() which will call it's own strlen() on the provided data. This patch also removes ldap_get_values() as a PHP function and makes it an alias of ldap_get_values_len() since there's no difference and the same data can be returned, it's just a safer version. The attached patch fixes the test case provided in bug #38819. Referencing for my own purposes: http://bugs.gentoo.org/show_bug.cgi?id=133467 ------=_20070228153310_59904 Content-Type: text/x-patch; name="php-5.1.6-ldap-api-fix.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="php-5.1.6-ldap-api-fix.patch" diff -Nur php-5.1.6/ext/ldap/ldap.c php-5.1.6-ldap-api/ext/ldap/ldap.c --- php-5.1.6/ext/ldap/ldap.c 2006-01-01 07:50:08.000000000 -0500 +++ php-5.1.6-ldap-api/ext/ldap/ldap.c 2007-02-28 11:04:05.000000000 -0500 @@ -116,7 +116,8 @@ PHP_FE(ldap_first_attribute, third_arg_force_ref) PHP_FE(ldap_next_attribute, third_arg_force_ref) PHP_FE(ldap_get_attributes, NULL) - PHP_FE(ldap_get_values, NULL) + PHP_FALIAS(ldap_get_values, ldap_get_values_len, NULL) +/* PHP_FE(ldap_get_values, NULL) */ PHP_FE(ldap_get_values_len, NULL) PHP_FE(ldap_get_dn, NULL) PHP_FE(ldap_explode_dn, NULL) @@ -1033,7 +1034,7 @@ BerElement *ber; char *attribute; size_t attr_len; - char **ldap_value; + struct berval **ldap_value; char *dn; if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &link, &result) == FAILURE) { @@ -1064,16 +1065,16 @@ attribute = ldap_first_attribute(ldap, ldap_result_entry, &ber); while (attribute != NULL) { - ldap_value = ldap_get_values(ldap, ldap_result_entry, attribute); - num_values = ldap_count_values(ldap_value); + ldap_value = ldap_get_values_len(ldap, ldap_result_entry, attribute); + num_values = ldap_count_values_len(ldap_value); MAKE_STD_ZVAL(tmp2); array_init(tmp2); add_assoc_long(tmp2, "count", num_values); for (i = 0; i < num_values; i++) { - add_index_string(tmp2, i, ldap_value[i], 1); + add_index_stringl(tmp2, i, ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1); } - ldap_value_free(ldap_value); + ldap_value_free_len(ldap_value); attr_len = strlen(attribute); zend_hash_update(Z_ARRVAL_P(tmp1), php_strtolower(attribute, attr_len), attr_len+1, (void *) &tmp2, sizeof(zval *), NULL); @@ -1180,7 +1181,7 @@ ldap_linkdata *ld; ldap_resultentry *resultentry; char *attribute; - char **ldap_value; + struct berval **ldap_value; int i, num_values, num_attrib; BerElement *ber; @@ -1196,16 +1197,16 @@ attribute = ldap_first_attribute(ld->link, resultentry->data, &ber); while (attribute != NULL) { - ldap_value = ldap_get_values(ld->link, resultentry->data, attribute); - num_values = ldap_count_values(ldap_value); + ldap_value = ldap_get_values_len(ld->link, resultentry->data, attribute); + num_values = ldap_count_values_len(ldap_value); MAKE_STD_ZVAL(tmp); array_init(tmp); add_assoc_long(tmp, "count", num_values); for (i = 0; i < num_values; i++) { - add_index_string(tmp, i, ldap_value[i], 1); + add_index_stringl(tmp, i, ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1); } - ldap_value_free(ldap_value); + ldap_value_free_len(ldap_value); zend_hash_update(Z_ARRVAL_P(return_value), attribute, strlen(attribute)+1, (void *) &tmp, sizeof(zval *), NULL); add_index_string(return_value, num_attrib, attribute, 1); @@ -1226,46 +1227,6 @@ } /* }}} */ -/* {{{ proto array ldap_get_values(resource link, resource result_entry, string attribute) - Get all values from a result entry */ -PHP_FUNCTION(ldap_get_values) -{ - zval **link, **result_entry, **attr; - ldap_linkdata *ld; - ldap_resultentry *resultentry; - char *attribute; - char **ldap_value; - int i, num_values; - - if (ZEND_NUM_ARGS() != 3 || zend_get_parameters_ex(3, &link, &result_entry, &attr) == FAILURE) { - WRONG_PARAM_COUNT; - } - - ZEND_FETCH_RESOURCE(ld, ldap_linkdata *, link, -1, "ldap link", le_link); - ZEND_FETCH_RESOURCE(resultentry, ldap_resultentry *, result_entry, -1, "ldap result entry", le_result_entry); - - convert_to_string_ex(attr); - attribute = Z_STRVAL_PP(attr); - - if ((ldap_value = ldap_get_values(ld->link, resultentry->data, attribute)) == NULL) { - php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot get the value(s) of attribute %s", ldap_err2string(_get_lderrno(ld->link))); - RETURN_FALSE; - } - - num_values = ldap_count_values(ldap_value); - - array_init(return_value); - - for (i = 0; i