headers
Simon Fraser | 23 Oct 2008 14:33
Picon
Favicon

bug: Partial results from DB lookup causing parsing problems


Hi folks,

I'm newly subscribed to this list, and while I've searched for a mention
of this before and come up with nothing, I apologise if I'm duplicating
a report.

I'm using perdition-1.17.1 with perditiondb_ldap.  Due to the nature of
our site, some users will have a mailhost configured in here, and some
will not.  I am hoping to use the outgoing_server configuration option
as a default should the server return a partial response, since one of
our existing systems will break if we started adding this to all the
currently empty fields. 

This works if the user had a mailhost set, but was refusing to use the
outgoing_server option if they did not, instead attempting to connect to
the person's username, treating it as a hostname. Omitting the port
works whether the server is set or not. 

I have tracked down this problem to user_server_port_strn_assign in
getserver.c.  This function appears to be dual-purpose, used for both
parsing the outgoing_server option in the configuration file, and also
for parsing the string returned by dbserver_get.  Since these have a
slightly different format (hostname[:port] vs
username[<delimiter>hostname[:port]]), it gets confused when the string
returned from the DB doesn't contain a server.  It falls through into
its configuration parsing option at the end, setting (*usp)->server to
(*usp)->user (the start of the string), and so the connection attempts
gethostbyname(username).
(Continue reading)

Permalink | Reply | 6 comments |
headers
Simon Horman | 24 Oct 2008 00:33
Picon
Gravatar

Re: bug: Partial results from DB lookup causing parsing problems

On Thu, Oct 23, 2008 at 01:33:09PM +0100, Simon Fraser wrote:
> 
> Hi folks,
> 
> I'm newly subscribed to this list, and while I've searched for a mention
> of this before and come up with nothing, I apologise if I'm duplicating
> a report.
> 
> I'm using perdition-1.17.1 with perditiondb_ldap.  Due to the nature of
> our site, some users will have a mailhost configured in here, and some
> will not.  I am hoping to use the outgoing_server configuration option
> as a default should the server return a partial response, since one of
> our existing systems will break if we started adding this to all the
> currently empty fields. 
> 
> This works if the user had a mailhost set, but was refusing to use the
> outgoing_server option if they did not, instead attempting to connect to
> the person's username, treating it as a hostname. Omitting the port
> works whether the server is set or not. 
> 
> I have tracked down this problem to user_server_port_strn_assign in
> getserver.c.  This function appears to be dual-purpose, used for both
> parsing the outgoing_server option in the configuration file, and also
> for parsing the string returned by dbserver_get.  Since these have a
> slightly different format (hostname[:port] vs
> username[<delimiter>hostname[:port]]), it gets confused when the string
> returned from the DB doesn't contain a server.  It falls through into
> its configuration parsing option at the end, setting (*usp)->server to
> (*usp)->user (the start of the string), and so the connection attempts
> gethostbyname(username).
(Continue reading)

Permalink | Reply | 5 comments |
headers
Simon Horman | 26 Oct 2008 12:54
Picon
Gravatar

Re: bug: Partial results from DB lookup causing parsing problems

On Fri, Oct 24, 2008 at 09:33:09AM +1100, Simon Horman wrote:
> On Thu, Oct 23, 2008 at 01:33:09PM +0100, Simon Fraser wrote:
> > 
> > Hi folks,
> > 
> > I'm newly subscribed to this list, and while I've searched for a mention
> > of this before and come up with nothing, I apologise if I'm duplicating
> > a report.
> > 
> > I'm using perdition-1.17.1 with perditiondb_ldap.  Due to the nature of
> > our site, some users will have a mailhost configured in here, and some
> > will not.  I am hoping to use the outgoing_server configuration option
> > as a default should the server return a partial response, since one of
> > our existing systems will break if we started adding this to all the
> > currently empty fields. 
> > 
> > This works if the user had a mailhost set, but was refusing to use the
> > outgoing_server option if they did not, instead attempting to connect to
> > the person's username, treating it as a hostname. Omitting the port
> > works whether the server is set or not. 
> > 
> > I have tracked down this problem to user_server_port_strn_assign in
> > getserver.c.  This function appears to be dual-purpose, used for both
> > parsing the outgoing_server option in the configuration file, and also
> > for parsing the string returned by dbserver_get.  Since these have a
> > slightly different format (hostname[:port] vs
> > username[<delimiter>hostname[:port]]), it gets confused when the string
> > returned from the DB doesn't contain a server.  It falls through into
> > its configuration parsing option at the end, setting (*usp)->server to
> > (*usp)->user (the start of the string), and so the connection attempts
(Continue reading)

Permalink | Reply | 4 comments |
headers
Simon Fraser | 27 Oct 2008 12:25
Picon
Favicon

Re: bug: Partial results from DB lookup causing parsing problems


I initially hit 'reply' rather than 'reply to all', so I'll re-send this
to the list.

> I don't have an LDAP db handy to test against, so could
> you please test the following change for me to see if
> it helps your problem. If not, I'll dig deeper into the problem.

Many thanks for getting back to me so quickly.  I've patched a clean
version of 1.17.1, and tested. 

> +     if (returns[0])
> +             user_str = &returns[0];
> +     if (returns[1])
> +             server_str = &returns[1];
> +     if (returns[2])
> +             port_str = &returns[2];

The lookup was failing, and I've traced it to the lines above.  These
pointers are directed to &returns, but a few lines later, all the values
of returns are passed to free():

        for (count = 0; count < attrcount; count++)
                if (returns[count] != NULL)
                        free(returns[count]);
        free(returns);

I've replaced the lines quoted above with ones like:

if (returns[1])
(Continue reading)

Permalink | Reply | 3 comments |
headers
Simon Horman | 28 Oct 2008 01:47
Picon
Gravatar

Re: bug: Partial results from DB lookup causing parsing problems

On Mon, Oct 27, 2008 at 11:25:40AM +0000, Simon Fraser wrote:
> 
> I initially hit 'reply' rather than 'reply to all', so I'll re-send this
> to the list.
> 
> > I don't have an LDAP db handy to test against, so could
> > you please test the following change for me to see if
> > it helps your problem. If not, I'll dig deeper into the problem.
> 
> Many thanks for getting back to me so quickly.  I've patched a clean
> version of 1.17.1, and tested. 
> 
> > +     if (returns[0])
> > +             user_str = &returns[0];
> > +     if (returns[1])
> > +             server_str = &returns[1];
> > +     if (returns[2])
> > +             port_str = &returns[2];
> 
> The lookup was failing, and I've traced it to the lines above.  These
> pointers are directed to &returns, but a few lines later, all the values
> of returns are passed to free():
> 
>         for (count = 0; count < attrcount; count++)
>                 if (returns[count] != NULL)
>                         free(returns[count]);
>         free(returns);
> 
> I've replaced the lines quoted above with ones like:
>
(Continue reading)

Permalink | Reply | 2 comments |
headers
Simon Fraser | 28 Oct 2008 13:53
Picon
Favicon

Re: bug: Partial results from DB lookup causing parsing problems

On Tue, 2008-10-28 at 11:47 +1100, Simon Horman wrote:

> Sorry about that. I think that your fix is ok, though
> I think I will go with the change below that just makes
> sure the strings aren't freed if we make it to the bottom
> cleanly.
> 
> The key is the following snippet
> 
>        leave:
> -       if (returns) {
> +       if (returns && status) {
>                 for (count = 0; count < attrcount; count++)

That does seem a lot cleaner - thank you. 

> > Incidentally, I've ventured into nasty kludge territory to work around a
> > bug that appears not to be in your code, but either in libldap or
> > libldap's interaction with Active Directory.  ldap_init, ldap_set_option
> > and ldap_bind_s are all successful, but ldap_search_s does not return
> > LDAP_SUCCESS.  Instead, it returns LDAP_SERVER_DOWN, _even if_ the
> > search was successful. It seems insane, but if I comment out the check
> > for (err != LDAP_SUCCESS) and just check for returned attributes, all
> > the data is there.  I can replicate this in my own test program using
> > libldap both from debian-etch (2.1.30) and the openldap-2.4.11 source
> > package, so I'm confident the problem isn't in your code, but it's
> > probably worth knowing about.
> 
> That does sound bizarre - but believable.
>
(Continue reading)

Permalink | Reply | 1 comment |
headers
Simon Horman | 31 Oct 2008 03:25
Picon
Gravatar

Re: bug: Partial results from DB lookup causing parsing problems

On Tue, Oct 28, 2008 at 12:53:00PM +0000, Simon Fraser wrote:
> On Tue, 2008-10-28 at 11:47 +1100, Simon Horman wrote:
> 
> > Sorry about that. I think that your fix is ok, though
> > I think I will go with the change below that just makes
> > sure the strings aren't freed if we make it to the bottom
> > cleanly.
> > 
> > The key is the following snippet
> > 
> >        leave:
> > -       if (returns) {
> > +       if (returns && status) {
> >                 for (count = 0; count < attrcount; count++)
> 
> That does seem a lot cleaner - thank you. 

Thanks, I have applied the change to hg.

http://hg.vergenet.net/perdition/perdition/rev/37c6aca9ee42

> > > Incidentally, I've ventured into nasty kludge territory to work around a
> > > bug that appears not to be in your code, but either in libldap or
> > > libldap's interaction with Active Directory.  ldap_init, ldap_set_option
> > > and ldap_bind_s are all successful, but ldap_search_s does not return
> > > LDAP_SUCCESS.  Instead, it returns LDAP_SERVER_DOWN, _even if_ the
> > > search was successful. It seems insane, but if I comment out the check
> > > for (err != LDAP_SUCCESS) and just check for returned attributes, all
> > > the data is there.  I can replicate this in my own test program using
> > > libldap both from debian-etch (2.1.30) and the openldap-2.4.11 source
(Continue reading)

Permalink | Reply | 0 comments |

Gmane