Howard Chu wrote: > Yoshinori Nishino wrote: >> Dear Howard-san, >> >> >> Thanks to your advice, I could create the attached tentative patches. >=20 > 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= s >> 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. >=20 > Never use calloc/malloc/free directly in slapd code. Always use=20 > ch_calloc/ch_malloc/ch_free. >=20 > In this particular case, there's no reason to allocate at all. Just def= ine=20 > "struct crypt_data data" as a local variable.
Also there's no reason to lock the passwd_mutex at all. That would comple= tely=20 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= e >>>> 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: >>>>> [email protected] wrote: >>>>>> Full_Name: Yoshinori Nishino >>>>>> Version: 2.4.45 >>>>>> OS: CentOS 7 >>>>>> URL: ftp://ftp.openldap.org/incoming/ >>>>>> Submission from: (NULL) (210.143.35.20) >>>>>> >>>>>> >>>>>> Dear sir, >>>>>> >>>>>> The function slapd_crypt() in servers/slapd/passwd.c seems to beco= me >>>>>> 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= apd? >>>>> >>>>> 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= t >>>>> test, since crypt_r is a non-standard function. >>>>>> >> >> >=20 >=20 --=20 -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
