[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#8719) slapd_crypt() become slow when many ldap cliant connections occur.

Howard Chu wrote:
> Yoshinori Nishino wrote:
>> Dear Howard-san,
>> Thanks to your advice, I could create the attached tentative patches.
> Thanks, this looks much better.
>> They modifies servers/slapd/passwd.c and configure.in
>> so that running "./configure" can check whether or not crypt_r() exist=
>> and use the function in slapd_crypt() if possible.
>> I am going to check whether or not "calloc()/free()" causes
>> unacceptable=E3=80=80memory fragmentation.
> Never use calloc/malloc/free directly in slapd code. Always use=20
> ch_calloc/ch_malloc/ch_free.
> In this particular case, there's no reason to allocate at all. Just def=
> "struct crypt_data data" as a local variable.

Also there's no reason to lock the passwd_mutex at all. That would comple=
defeat the purpose of using crypt_r() in the first place.
>> Best Regards,
>> ------------------------------------------------
>> On 2017/09/01 20:23, Yoshinori Nishino wrote:
>>> On 2017/08/30 20:46, Yoshinori Nishino wrote:
>>>> Dear Howard-san,
>>>> Thank you so much for your prompt advice.
>>>> As you told, I need to check whether or not crypt_r() exists.
>>>> I am going to consider including the check.
>>>> Would it be possible to let me know whether there are any other
>>>> concerns about the fix if we can use crypt_r()?
>>>> The implementation that "both calloc() and free() occur every time
>>>> slapd_crypt() is called" does not seem to look good.
>>>> The callers of slapd_crypt() might be able to get the memory for
>>>> "struct crypt_data" before they call it.
>>>> However, I think it causes the changes of other source codes.
>>>> So, I think the aforementioned implementation is a workaround for th=
>>>> slowdown if the risk of memory fragmentation can be acceptable.
>>>> On the other hands, I think we do not need to care about strcmp()
>>>> because it is thread-safe.
>>>> Best Regards,
>>>> ----------------------------------------
>>>> On 2017/08/30 19:26, Howard Chu wrote:
>>>>> yos-nishino@ys.jp.nec.com wrote:
>>>>>> Full_Name: Yoshinori Nishino
>>>>>> Version: 2.4.45
>>>>>> OS: CentOS 7
>>>>>> URL: ftp://ftp.openldap.org/incoming/
>>>>>> Submission from: (NULL) (
>>>>>> Dear sir,
>>>>>> The function slapd_crypt() in servers/slapd/passwd.c seems to beco=
>>>>>> slow when
>>>>>> many ldap client connections occur.
>>>>>> It seems it is because the function uses crypt()(non thread-safe
>>>>>> function) and
>>>>>> pthread_mutex_lock(), which results in the slowdown.
>>>>>> #Besides, we need to use {CRYPT} hash as users' password hash.
>>>>>> So, I modified servers/slapd/passwd.c like the following.
>>>>>> As a result, slapd_crypt() becomes much faster under the same
>>>>>> condition.
>>>>>> Would you let me know whether or not the fix is appropriate for sl=
>>>>> No it is not an appropriate fix.
>>>>> You should add an autoconf test to check for the existence of the
>>>>> crypt_r function, and use an #ifdef here based on the result of tha=
>>>>> test, since crypt_r is a non-standard function.

   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/