Re: [HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-12 Thread Albe Laurenz
Tom Lane wrote:

> You missed one "return" where the string needed to be freed.  I've
> applied this patch with that fix and a couple of cosmetic changes.
> Thanks for the report and patch!

Thanks for the work and the keen eye!

Yours,
Laurenz Albe

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-12 Thread Tom Lane
"Albe Laurenz"  writes:
> I have attached a new version of the patch that should address all known
> problems.

You missed one "return" where the string needed to be freed.  I've
applied this patch with that fix and a couple of cosmetic changes.
Thanks for the report and patch!

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-12 Thread Albe Laurenz
Tom Lane wrote:
>> I have found a small but annoying bug in libpq where
>> connection parameters are resolved via LDAP.

> Hmm ... that's a bug all right, but why have the null-termination
> inside the loop at all?  Seems like it should look like
>
>   for (p = result, i = 0; values[i] != NULL; ++i)
>   {
>   strncpy(p, values[i]->bv_val, values[i]->bv_len);
>   p += values[i]->bv_len;
>   *(p++) = '\n';
>   }
>   *p = '\0';

Yes, that's better.

> ... btw, shouldn't this function free the "result" string when it's
done
> with it?  AFAICS that string is not returned to the caller, it's just
> being leaked.

Oops, yes it should.

> (I'll refrain from asking why it's creating the string in the first
> place rather than parsing ldap_get_values_len's output as-is ...)

So that I can close the LDAP connection as soon as feasible, but of
course
that's not absolutely necessary.

I have attached a new version of the patch that should address all known
problems.

Yours,
Laurenz Albe


ldapServiceLookup.patch
Description: ldapServiceLookup.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-11 Thread Tom Lane
... btw, shouldn't this function free the "result" string when it's done
with it?  AFAICS that string is not returned to the caller, it's just
being leaked.

(I'll refrain from asking why it's creating the string in the first
place rather than parsing ldap_get_values_len's output as-is ...)

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-11 Thread Tom Lane
"Albe Laurenz"  writes:
> I have found a small but annoying bug in libpq where
> connection parameters are resolved via LDAP.

> There is a write past the end of a malloc'ed string which causes
> memory corruption. The code and the bug are originally by me :^(

Hmm ... that's a bug all right, but why have the null-termination
inside the loop at all?  Seems like it should look like

for (p = result, i = 0; values[i] != NULL; ++i)
{
strncpy(p, values[i]->bv_val, values[i]->bv_len);
p += values[i]->bv_len;
*(p++) = '\n';
}
*p = '\0';

> This should be backpatched to 8.2 where the code was introduced.

Yes, will do.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Fix for bug in ldapServiceLookup in libpq

2011-05-11 Thread Albe Laurenz
I have found a small but annoying bug in libpq where
connection parameters are resolved via LDAP.

There is a write past the end of a malloc'ed string which causes
memory corruption. The code and the bug are originally by me :^(

The attached patch fixes the problem in HEAD.
This should be backpatched to 8.2 where the code was introduced.

Yours,
Laurenz Albe


ldapServiceLookup.patch
Description: ldapServiceLookup.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers