332159 - ldap_getnextfilter returns corrupt filter (need to strdup value)

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[Jeff Paquette]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: 5.1.7

Given the following two filters in the ldap filter.conf file:
  "@"		    " "	   "(&(objectClass=person)(mail=%v))"					   "matches email address"
			   "(&(objectClass=person)(mail=%v*))"					   "start of email address"

a user-initiated search for 'jill' generates the following filters:
ldap_getfirstfilter returns '(&(objectClass=person)(mail=jill@))'
ldap_getnextfilter returns '(&(objectClass=person)(mail=оюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюо'

The garbage in the second filter varies from run to run.
The filter returned by ldap_getnextfilter seems to be corrupt no matter what the filter is.

Reproducible: Always

Actual Results:  
ldap_getnextfilter returns '(&(objectClass=person)(mail=оюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюоюо'


Expected Results:  
ldap_getnextfilter should return (&(objectClass=person)(mail=jill@*))'


Originally found in Sun C SDK 5.08. Rebuilt our application with Mozilla LDAP C SDK 5.1.7 with the same results. Diff'd the file getfilter.c from both sdks; the files are identical.

Comment 1

[Anton Bobrov]

do you think you can provide any tescase for this?
i'm unable to reproduce what you have described
and tend to think you have a bug in your code
actually. getfilt.c from examples folder is
something you can modify and test with.

Comment 2

[Jeff Paquette]

I compiled and ran getfilt.c and it worked as advertised. I went back to my code and one significant difference in the calls to ldap_getfirstfilter() is that the tagpat and value pointers were temporary and did not exist when the call to ldap_getnextfilter() was made. As a test (meaning I can't ship the code this way :) ) I altered the lifetime of the these two pointers so that they would be valid when I called ldap_getnextfilter() and the corruption I was seeing went away.

I would suggest that either ldap_getfirstfilter() make a local copy of *tagpat and *value if it requires them when ldap_getnextfilter() is called or the documentation be updated to describe the lifetime requirements of the parameters.

As an aside, what are the restrictions on the filter-tag in the filter file? I've found that "auth-iplanet" and "auth-active-directory" both were resolved to the first occurance in the filter file, it did not matter which came first.

Comment 3

[Mark Smith [:mcs]]

(In reply to comment #2)
> ...
> I would suggest that either ldap_getfirstfilter() make a local copy of *tagpat
> and *value if it requires them when ldap_getnextfilter() is called or the
> documentation be updated to describe the lifetime requirements of the
> parameters.

Thanks for tracking down the problem (and thanks to Anton for his efforts too)!  Looking at the code, I think that this problem could be fixed by strdup'ing the value parameter inside ldap_getfirstfilter().  I don't think a pointer to tagpat is kept anywhere, so we should be OK for tagpat.  As a workaround, in your code you could define your own struct that holds both a pointer to the LDAPFilterDesc that is returned by ldap_init_getfilter() and a copy of your "value" value.  When you are done using ldap_getnextfilter() or when you call ldap_getfirstfilter() a 2nd time, free the old "value", strdup the new one and tuck it away.

> As an aside, what are the restrictions on the filter-tag in the filter file?
> I've found that "auth-iplanet" and "auth-active-directory" both were resolved
> to the first occurance in the filter file, it did not matter which came first.

The filter tags are regular expressions and matching is attempted in "top to bottom" order within the filter configuration data.  I am not sure exactly what you are doing, but you might be able to use ^ and $ to ensure that the entire string must match, e.g.,

  ^auth-iplanet$
     ...
  ^auth-active-directory$"
     ...

I have not tried this myself though.

Comment 4

[Mark Smith [:mcs]]

I updated the bug Summary to mention the root cause of this problem.

Comment 5

[Rich Megginson]

Attachment: diffs for fix

ldap_getfirstfilter will now strdup the given value as the curval, first checking to see if curval is not null and freeing it if it is.  Also free the curval in the free function.

Comment 6

[Mark Smith [:mcs]]

Comment on attachment 221711 [details]
diffs for fix

The patch looks good.  Has it been tested?

Comment 7

[Rich Megginson]

No, I haven't tested it.  I'm not really sure how to set it up.  I'll give it a try.  Jeff, any chance you could test the proposed patch?

Comment 8

[Rich Megginson]

Can you test the patch, or provide instructions for setting up an environment for testing?  We have a window of opportunity for a couple of weeks to do a lot of work on the ldap c sdk.  Once we get past that window, not much work is going to get done.  So it is very important to respond to get this fixed asap.

Comment 9

[Jeff Paquette]

I don't have the cycles or the environment to test the patch. Here is a small testcase that could be incorporated into getfilt.c:

...
    /* search for what the user typed */
    found = 0;
    for ( ldfi = ldap_getfirstfilter( ldfp, MY_FILTERTAG, buf );
	    ldfi != NULL; ldfi = ldap_getnextfilter( ldfp )) {
	printf( "Filter: \"%s\"... ", ldfi->lfi_filter );
.. change to...
    /* search for what the user typed */
    found = 0;
    char *tag = strdup(MY_FILTERTAG);
    char *pat = strdup(buf);
    for ( ldfi = ldap_getfirstfilter( ldfp, MY_FILTERTAG, buf );
	    ldfi != NULL; ldfi = ldap_getnextfilter( ldfp )) {
        if (tag) { free (tag); tag = NULL);
        if (pat) { free (pat); pat = NULL);
	printf( "Filter: \"%s\"... ", ldfi->lfi_filter );


This simulates what my code was doing. To test the dangling pointer issue, remove the assignment of null to the pointers.,

Comment 10

[Rich Megginson]

I was able to test it.  Thanks Jeff!

Checking in getfilter.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/getfilter.c,v  <--  getfilter.c
new revision: 5.5; previous revision: 5.4
done
Checking in free.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/free.c,v  <--  free.c
new revision: 5.3; previous revision: 5.2
done

Comment 11

[Rich Megginson]

Mark, you can close this.

Comment 12

[Mark Smith [:mcs]]

Marking fixed.  Thanks Rich and Jeff!