You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Nick Kew <ni...@webthing.com> on 2008/03/18 23:01:37 UTC
Re: svn commit: r634821 - in /httpd/httpd/trunk: CHANGES
modules/ldap/util_ldap.c
On Fri, 07 Mar 2008 21:02:42 -0000
covener@apache.org wrote:
> Modified: httpd/httpd/trunk/modules/ldap/util_ldap.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ldap/util_ldap.c?rev=634821&r1=634820&r2=634821&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ldap/util_ldap.c (original) +++
> httpd/httpd/trunk/modules/ldap/util_ldap.c Fri Mar 7 13:02:41 2008
> @@ -1462,13 +1462,10 @@ /* ...and entry is valid */
> *binddn = apr_pstrdup(r->pool, search_nodep->dn);
> if (attrs) {
> - int i = 0, k = 0;
> - while (attrs[k++]);
> - *retvals = apr_pcalloc(r->pool, sizeof(char *) *
> k);
> - while (search_nodep->vals[i]) {
> - (*retvals)[i] = apr_pstrdup(r->pool,
> -
> search_nodep->vals[i]);
> - i++;
> + int i;
> + *retvals = apr_pcalloc(r->pool, sizeof(char *) *
> search_nodep->numvals);
> + for (i = 0; i < search_nodep->numvals; i++) {
> + (*retvals)[i] = apr_pstrdup(r->pool,
> search_nodep->vals[i]); }
Um, doesn't that need a not-null test to avoid a segfault?
If the switch from while(...) to for(...) does anything,
it must be 'cos there's a null in there, yesno?
Also, because I CBA to figure it out, I take it the
search_nodep->vals[i] must live on the stack and/or get
overwritten, so the copying is necessary?
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: svn commit: r634821 - in /httpd/httpd/trunk: CHANGES modules/ldap/util_ldap.c
Posted by Eric Covener <co...@gmail.com>.
On Tue, Mar 18, 2008 at 6:01 PM, Nick Kew <ni...@webthing.com> wrote:
> On Fri, 07 Mar 2008 21:02:42 -0000
> covener@apache.org wrote:
>
>
> > Modified: httpd/httpd/trunk/modules/ldap/util_ldap.c
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ldap/util_ldap.c?rev=634821&r1=634820&r2=634821&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/ldap/util_ldap.c (original) +++
> > httpd/httpd/trunk/modules/ldap/util_ldap.c Fri Mar 7 13:02:41 2008
> > @@ -1462,13 +1462,10 @@ /* ...and entry is valid */
> > *binddn = apr_pstrdup(r->pool, search_nodep->dn);
> > if (attrs) {
> > - int i = 0, k = 0;
> > - while (attrs[k++]);
> > - *retvals = apr_pcalloc(r->pool, sizeof(char *) *
> > k);
> > - while (search_nodep->vals[i]) {
> > - (*retvals)[i] = apr_pstrdup(r->pool,
> > -
> > search_nodep->vals[i]);
> > - i++;
> > + int i;
> > + *retvals = apr_pcalloc(r->pool, sizeof(char *) *
> > search_nodep->numvals);
> > + for (i = 0; i < search_nodep->numvals; i++) {
> > + (*retvals)[i] = apr_pstrdup(r->pool,
> > search_nodep->vals[i]); }
>
> Um, doesn't that need a not-null test to avoid a segfault?
> If the switch from while(...) to for(...) does anything,
> it must be 'cos there's a null in there, yesno?
Yes, vals[] can be swiss-cheese and the old loop would stop processing
early (attrs[], not vals[], is null terminated per the ldap API)
apr_pstrdup() accomodates for the null parameter down to .9.x
>
> Also, because I CBA to figure it out, I take it the
> search_nodep->vals[i] must live on the stack and/or get
> overwritten, so the copying is necessary?
The copying is necessary because these strings live in the shared
memory util_ldap/mod_ldap cache. The caller (authnz_ldap) doesn't
directly hold a cache lock, so there is no chance later on to copy
this info out safely.
--
Eric Covener
covener@gmail.com