--_003_282E583D7ED52C4FA3C4CC199465868B02801BECBPXM04GPgispnec_
Content-Type: text/plain; charset="iso-2022-jp"
Content-Transfer-Encoding: quoted-printable
Dear Howard-san,
I appreciate your many valuable advices.
>Never use calloc/malloc/free directly in slapd code. Always use ch_calloc/=
ch_malloc/ch_free.
>
>In this particular case, there's no reason to allocate at all. Just define=
"struct crypt_data data" as a local variable.
You are right. There is no need to allocate memory …
[View More]for "data" because using=
a local variable is also thread-safe in this case.
>Also there's no reason to lock the passwd_mutex at all. That would complet=
ely defeat the purpose of using crypt_r() in the first place.
I decide to use passwd_mutex only for ber_strdup(), which seems to be non =
thread-safe according to ./libraries/liblber/memory.c.
I created two patches for passwd.c.
Both patches uses "data" as a local variable. =20
"case1" uses passwd_mutex for ber_strdup(), "case2" dose not so.
For the time being , I am going to use "case1" due to the aforementioned th=
read-safe concern about ber_strdup().
Best Regards,
************************************************
Yoshinori Nishino
NEC Solution Innovators, Ltd.
1-18-7 Shinkiba, Koto-ku, Tokyo, 136-8627 Japan
E-MAIL: yos-nishino(a)ys.jp.nec.com
************************************************
--_003_282E583D7ED52C4FA3C4CC199465868B02801BECBPXM04GPgispnec_
Content-Type: application/octet-stream;
name="openldap-slapd_crypt_case1.patch"
Content-Description: openldap-slapd_crypt_case1.patch
Content-Disposition: attachment;
filename="openldap-slapd_crypt_case1.patch"; size=1345;
creation-date="Sun, 03 Sep 2017 01:05:32 GMT";
modification-date="Sun, 03 Sep 2017 01:08:20 GMT"
Content-Transfer-Encoding: base64
bW9kaWZ5IHBhc3N3ZC5jIHNvIHRoYXQgc2xhcGRfY3J5cHQoKSB1c2VzIGNyeXB0X3IoKS4KCmRp
ZmYgLS1naXQgYS9wYXNzd2QuYy4gYi9wYXNzd2QuYwppbmRleCBkZmEzNzBjLi5hYzA3ZGQyIDEw
MDY0NAotLS0gYS9wYXNzd2QuYworKysgYi9wYXNzd2QuYwpAQCAtMjMsOCArMjMsMTEgQEAKICNp
bmNsdWRlIDxhYy91bmlzdGQuaD4KIAogI2lmZGVmIFNMQVBEX0NSWVBUCisjaWZkZWYgSEFWRV9D
UllQVF9SCisjZGVmaW5lIF9fVVNFX0dOVQorI2VuZGlmIC8qIEhBVkVfQ1JZUFRfUiAqLwogI2lu
Y2x1ZGUgPGFjL2NyeXB0Lmg+Ci0jZW5kaWYKKyNlbmRpZiAvKiBTTEFQRF9DUllQVCAqLwogCiAj
aW5jbHVkZSAic2xhcC5oIgogCkBAIC01OTAsNiArNTkzLDMxIEBAIHNsYXBfcGFzc3dkX2hhc2go
CiBzdGF0aWMgbGRhcF9wdnRfdGhyZWFkX211dGV4X3QgcGFzc3dkX211dGV4Owogc3RhdGljIGx1
dGlsX2NyeXB0ZnVuYyBzbGFwZF9jcnlwdDsKIAorI2lmZGVmIEhBVkVfQ1JZUFRfUgorc3RhdGlj
IGludCBzbGFwZF9jcnlwdCggY29uc3QgY2hhciAqa2V5LCBjb25zdCBjaGFyICpzYWx0LCBjaGFy
ICoqaGFzaCApCit7CisJY2hhciAqY3I7CisJaW50IHJjOworCXN0cnVjdCBjcnlwdF9kYXRhIGRh
dGE7CisgICAgCisJZGF0YS5pbml0aWFsaXplZCA9IDA7CisJY3IgPSBjcnlwdF9yKCBrZXksIHNh
bHQsICZkYXRhICk7CisJaWYgKCBjciA9PSBOVUxMIHx8IGNyWzBdID09ICdcMCcgKSB7CisJCS8q
IHNhbHQgbXVzdCBoYXZlIGJlZW4gaW52YWxpZCAqLworCQlyYyA9IExVVElMX1BBU1NXRF9FUlI7
CisJfSBlbHNlIHsKKwkJaWYgKCBoYXNoICkgeworCQkJKmhhc2ggPSBiZXJfc3RyZHVwKCBjciAp
OworCQkJcmMgPSBMVVRJTF9QQVNTV0RfT0s7CisJCX0gZWxzZSB7CisJCQlyYyA9IHN0cmNtcCgg
c2FsdCwgY3IgKSA/IExVVElMX1BBU1NXRF9FUlIgOiBMVVRJTF9QQVNTV0RfT0s7CisJCX0KKwl9
CisgCisgICAgZnJlZShkYXRhKTsKKyAgICByZXR1cm4gcmM7Cit9CisjZWxzZQogc3RhdGljIGlu
dCBzbGFwZF9jcnlwdCggY29uc3QgY2hhciAqa2V5LCBjb25zdCBjaGFyICpzYWx0LCBjaGFyICoq
aGFzaCApCiB7CiAJY2hhciAqY3I7CkBAIC02MTQsNiArNjQyLDggQEAgc3RhdGljIGludCBzbGFw
ZF9jcnlwdCggY29uc3QgY2hhciAqa2V5LCBjb25zdCBjaGFyICpzYWx0LCBjaGFyICoqaGFzaCAp
CiAJbGRhcF9wdnRfdGhyZWFkX211dGV4X3VubG9jayggJnBhc3N3ZF9tdXRleCApOwogCXJldHVy
biByYzsKIH0KKyNlbmRpZiAvKiBIQVZFX0NSWVBUX1IgKi8KKwogI2VuZGlmIC8qIFNMQVBEX0NS
WVBUICovCiAKIHZvaWQgc2xhcF9wYXNzd2RfaW5pdCgpCg==
--_003_282E583D7ED52C4FA3C4CC199465868B02801BECBPXM04GPgispnec_
Content-Type: application/octet-stream;
name="openldap-slapd_crypt_case2.patch"
Content-Description: openldap-slapd_crypt_case2.patch
Content-Disposition: attachment;
filename="openldap-slapd_crypt_case2.patch"; size=1444;
creation-date="Sun, 03 Sep 2017 01:05:35 GMT";
modification-date="Sun, 03 Sep 2017 01:08:20 GMT"
Content-Transfer-Encoding: base64
bW9kaWZ5IHBhc3N3ZC5jIHNvIHRoYXQgc2xhcGRfY3J5cHQoKSB1c2VzIGNyeXB0X3IoKS4KCmRp
ZmYgLS1naXQgYS9wYXNzd2QuYyBiL3Bhc3N3ZC5jCmluZGV4IGRmYTM3MGMuLjM1ZDg3ZTUgMTAw
NjQ0Ci0tLSBhL3Bhc3N3ZC5jCisrKyBiL3Bhc3N3ZC5jCkBAIC0yMyw4ICsyMywxMSBAQAogI2lu
Y2x1ZGUgPGFjL3VuaXN0ZC5oPgogCiAjaWZkZWYgU0xBUERfQ1JZUFQKKyNpZmRlZiBIQVZFX0NS
WVBUX1IKKyNkZWZpbmUgX19VU0VfR05VCisjZW5kaWYgLyogSEFWRV9DUllQVF9SICovCiAjaW5j
bHVkZSA8YWMvY3J5cHQuaD4KLSNlbmRpZgorI2VuZGlmIC8qIFNMQVBEX0NSWVBUICovCiAKICNp
bmNsdWRlICJzbGFwLmgiCiAKQEAgLTU5MCw2ICs1OTMsMzMgQEAgc2xhcF9wYXNzd2RfaGFzaCgK
IHN0YXRpYyBsZGFwX3B2dF90aHJlYWRfbXV0ZXhfdCBwYXNzd2RfbXV0ZXg7CiBzdGF0aWMgbHV0
aWxfY3J5cHRmdW5jIHNsYXBkX2NyeXB0OwogCisjaWZkZWYgSEFWRV9DUllQVF9SCitzdGF0aWMg
aW50IHNsYXBkX2NyeXB0KCBjb25zdCBjaGFyICprZXksIGNvbnN0IGNoYXIgKnNhbHQsIGNoYXIg
KipoYXNoICkKK3sKKwljaGFyICpjcjsKKwlpbnQgcmM7CisJc3RydWN0IGNyeXB0X2RhdGEgZGF0
YTsKKyAgICAKKwlkYXRhLmluaXRpYWxpemVkID0gMDsKKwljciA9IGNyeXB0X3IoIGtleSwgc2Fs
dCwgJmRhdGEgKTsKKwlpZiAoIGNyID09IE5VTEwgfHwgY3JbMF0gPT0gJ1wwJyApIHsKKwkJLyog
c2FsdCBtdXN0IGhhdmUgYmVlbiBpbnZhbGlkICovCisJCXJjID0gTFVUSUxfUEFTU1dEX0VSUjsK
Kwl9IGVsc2UgeworCQlpZiAoIGhhc2ggKSB7CisJCQlsZGFwX3B2dF90aHJlYWRfbXV0ZXhfbG9j
ayggJnBhc3N3ZF9tdXRleCApOworCQkJKmhhc2ggPSBiZXJfc3RyZHVwKCBjciApOworCQkJbGRh
cF9wdnRfdGhyZWFkX211dGV4X3VubG9jayggJnBhc3N3ZF9tdXRleCApOworCQkJcmMgPSBMVVRJ
TF9QQVNTV0RfT0s7CisJCX0gZWxzZSB7CisJCQlyYyA9IHN0cmNtcCggc2FsdCwgY3IgKSA/IExV
VElMX1BBU1NXRF9FUlIgOiBMVVRJTF9QQVNTV0RfT0s7CisJCX0KKwl9CisgCisgICAgZnJlZShk
YXRhKTsKKyAgICByZXR1cm4gcmM7Cit9CisjZWxzZQogc3RhdGljIGludCBzbGFwZF9jcnlwdCgg
Y29uc3QgY2hhciAqa2V5LCBjb25zdCBjaGFyICpzYWx0LCBjaGFyICoqaGFzaCApCiB7CiAJY2hh
ciAqY3I7CkBAIC02MTQsNiArNjQ0LDggQEAgc3RhdGljIGludCBzbGFwZF9jcnlwdCggY29uc3Qg
Y2hhciAqa2V5LCBjb25zdCBjaGFyICpzYWx0LCBjaGFyICoqaGFzaCApCiAJbGRhcF9wdnRfdGhy
ZWFkX211dGV4X3VubG9jayggJnBhc3N3ZF9tdXRleCApOwogCXJldHVybiByYzsKIH0KKyNlbmRp
ZiAvKiBIQVZFX0NSWVBUX1IgKi8KKwogI2VuZGlmIC8qIFNMQVBEX0NSWVBUICovCiAKIHZvaWQg
c2xhcF9wYXNzd2RfaW5pdCgpCg==
--_003_282E583D7ED52C4FA3C4CC199465868B02801BECBPXM04GPgispnec_--
[View Less]
On 09/01/2017 02:38 PM, Howard Chu wrote:
> vladimir.cunat(a)nic.cz wrote:
>> The documentation of txn_commit did confuse me (as well).
>> https://github.com/LMDB/lmdb/blob/LMDB_0.9.21/libraries/liblmdb/lmdb.h#L994
>>
>> Would you consider adding a line stressing that the txn gets released
>> even in case any error is returned? (or something similar) I'm
>> personally used to failing operations not "having externally observable
>> effects" unless …
[View More]explicitly stated.
>
> The very first words on the line you highlight are "The transaction
> handle is freed." I don't know how to state that any more explicitly.
Well, okay.
(I knew the line stated it and still it somehow didn't make me sure, but
it's perhaps just me.)
[View Less]
vladimir.cunat(a)nic.cz wrote:
> Hello!
>
>> The docs say quite clearly, after txn_commit the txn must not be used
> again.
>
> The documentation of txn_commit did confuse me (as well).
> https://github.com/LMDB/lmdb/blob/LMDB_0.9.21/libraries/liblmdb/lmdb.h#L994
> Would you consider adding a line stressing that the txn gets released
> even in case any error is returned? (or something similar) I'm
> personally used to failing operations not "having externally …
[View More]observable
> effects" unless explicitly stated.
The very first words on the line you highlight are "The transaction handle is
freed." I don't know how to state that any more explicitly.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
[View Less]
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()…
[View More]" 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:
>>>>> yos-nishino(a)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) (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/
[View Less]
Yoshinori Nishino wrote:
> Dear Howard-san,
>=20
>=20
> Thanks to your advice, I could create the attached tentative patches.
Thanks, this looks much better.
>=20
> They modifies servers/slapd/passwd.c and configure.in
> so that running "./configure" can check whether or not crypt_r() exists
> and use the function in slapd_crypt() if possible.
>=20
> I am going to check whether or not "calloc()/free()" causes
> unacceptable=E3=80=80memory fragmentation.
Never …
[View More]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 defin=
e=20
"struct crypt_data data" as a local variable.
>=20
>=20
> 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 the
>>> 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(a)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) (210.143.35.20)
>>>>>
>>>>>
>>>>> Dear sir,
>>>>>
>>>>> The function slapd_crypt() in servers/slapd/passwd.c seems to becom=
e
>>>>> 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 sla=
pd?
>>>>
>>>> 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 that
>>>> 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/
[View Less]
Hello!
> The docs say quite clearly, after txn_commit the txn must not be used
again.
The documentation of txn_commit did confuse me (as well).
https://github.com/LMDB/lmdb/blob/LMDB_0.9.21/libraries/liblmdb/lmdb.h#L994
Would you consider adding a line stressing that the txn gets released
even in case any error is returned? (or something similar) I'm
personally used to failing operations not "having externally observable
effects" unless explicitly stated.
Thanks.
--Vladimir