Full_Name: Leo Yuriev Version: 2.4-HEAD OS: RHEL7 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (31.130.36.33) 1) LMDB: On 64-bit systems, in some cases mdb_cmp_int() could be called instead of mdb_cmp_long(), when key.mv_size == 8. To reproduce this is enough add an assertion-check for key.size == sizeof(int) into mdb_cmp_int(). A%A 2) LMDB: There is no any checking for invalid key-size for MDB_INTEGERKEY and MDB_INTEGERDUP. Therefore, for example, mdb_cursor_put() accepts a key.size == 1 and so forth. 3) lmdb-backend: AttributeDescription->ad_type->sat_equality->smr_indexer() could generate a 5-byte sized ks.s. I can just imagine what this can create a many problems. For instance, when I adds the control for key size, it breaks test017-syncreplication-refresh. But I am not tested more. P.S. It is a one more nice rebus from Howard ;)
leo@yuriev.ru wrote: > Full_Name: Leo Yuriev > Version: 2.4-HEAD > OS: RHEL7 > URL: ftp://ftp.openldap.org/incoming/ > Submission from: (NULL) (31.130.36.33) > > > 1) LMDB: On 64-bit systems, in some cases mdb_cmp_int() could be called instead > of mdb_cmp_long(), when key.mv_size == 8. > To reproduce this is enough add an assertion-check for key.size == sizeof(int) > into mdb_cmp_int().A%A You need to provide more details than this. mdb_cmp_int() isn't even used on regular databases, only on DUPSORT databases. Provide actual code reproducing the issue. > 2) LMDB: There is no any checking for invalid key-size for MDB_INTEGERKEY and > MDB_INTEGERDUP. Therefore, for example, mdb_cursor_put() accepts a key.size == 1 > and so forth. Not a bug, MDB_INTEGERKEY is documented to only be used with word-sized keys. > 3) lmdb-backend: AttributeDescription->ad_type->sat_equality->smr_indexer() > could generate a 5-byte sized ks.s. I can just imagine what this can create a > many problems. For instance, when I adds the control for key size, it breaks > test017-syncreplication-refresh. But I am not tested more. Not a bug, indexer keys are padded to 8 bytes. > P.S. > It is a one more nice rebus from Howard ;) Thanks for the irrelevant editorials. It might even be amusing if they were true, but mostly they just demonstrate your inexperience. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
29.04.2015 16:06, Howard Chu пишет: > leo@yuriev.ru wrote: >> Full_Name: Leo Yuriev >> Version: 2.4-HEAD >> OS: RHEL7 >> URL: ftp://ftp.openldap.org/incoming/ >> Submission from: (NULL) (31.130.36.33) >> >> >> 1) LMDB: On 64-bit systems, in some cases mdb_cmp_int() could be >> called instead >> of mdb_cmp_long(), when key.mv_size == 8. >> To reproduce this is enough add an assertion-check for key.size == >> sizeof(int) >> into mdb_cmp_int().A%A > > You need to provide more details than this. mdb_cmp_int() isn't even > used on regular databases, only on DUPSORT databases. Provide actual > code reproducing the issue. > this is enough: - add assert(a->mv_key_size == sizeof(size_t)) into mdb_cmp_long() - make all test - see a coredump > >> 2) LMDB: There is no any checking for invalid key-size for >> MDB_INTEGERKEY and >> MDB_INTEGERDUP. Therefore, for example, mdb_cursor_put() accepts a >> key.size == 1 >> and so forth. > > Not a bug, MDB_INTEGERKEY is documented to only be used with > word-sized keys. Ok, let it be "a feature" ;) >> 3) lmdb-backend: >> AttributeDescription->ad_type->sat_equality->smr_indexer() >> could generate a 5-byte sized ks.s. I can just imagine what this can >> create a >> many problems. For instance, when I adds the control for key size, it >> breaks >> test017-syncreplication-refresh. But I am not tested more. > > Not a bug, indexer keys are padded to 8 bytes. There is just NOT padded, I am sure. Also this is reason for unaligned-access in mdb_cmp_int() and mdb_cmp_long(). It is similar to reproduce, just assert + make test. Seems that bugs together could break ordering of entries, this is detected by loop over all entries with mdb_cursor_get(..., MDB_NEXT) and comparing keys from previous step. All of this I was found today, during developing a mdb_chk tool. >> P.S. >> It is a one more nice rebus from Howard ;) > > Thanks for the irrelevant editorials. It might even be amusing if they > were true, but mostly they just demonstrate your inexperience. > Code style and code quality of OpenLDAP - is just a trash (IMHO). By functional is a 100 times simple than (for instance) Linux/FreeBSD/Windows kernel, or PostgreSQL. But simultaneously it is 100 times harder to find and fix bugs or make a some progress. You can't hide the obvious.
leo@yuriev.ru wrote: > Code style and code quality of OpenLDAP - is just a trash (IMHO). And your opinion is worth what, exactly? This code has come a long way since I started working on it in 1998. Frankly I had the same opinion then, as you do now. But rather than complain, I fixed the problems I found. There are probably still many more of course, in corners of the code I have never looked at. If you think you can do better, prove it - take a copy of UMich LDAP 3.3, where OpenLDAP started from, and bring it up to date with LDAPv3 and make it match OpenLDAP's performance and features. I look forward to reviewing your result. Until you do that, you have no basis for your pointless insults. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
29.04.2015 17:07, Howard Chu пишет: > leo@yuriev.ru wrote: >> Code style and code quality of OpenLDAP - is just a trash (IMHO). > > And your opinion is worth what, exactly? > > This code has come a long way since I started working on it in 1998. > Frankly I had the same opinion then, as you do now. But rather than > complain, I fixed the problems I found. There are probably still many > more of course, in corners of the code I have never looked at. > > If you think you can do better, prove it - take a copy of UMich LDAP > 3.3, where OpenLDAP started from, and bring it up to date with LDAPv3 > and make it match OpenLDAP's performance and features. I look forward > to reviewing your result. > > Until you do that, you have no basis for your pointless insults. > I have no intention to offend or humiliate you. It's hard to say what I would do in your position, but I have my opinion (as I believe informed) about the quality of the code. Whether are necessary to correct the situation, and how to do it - you decide. I think the first important step - to acknowledge the problem and put into a goals. For my part I can say that I must to achieve stability, at least in our scenarios. Regards, Leonid.
Leonid Yuriev wrote: > > > 29.04.2015 16:06, Howard Chu пишет: >> leo@yuriev.ru wrote: >>> Full_Name: Leo Yuriev >>> Version: 2.4-HEAD >>> OS: RHEL7 >>> URL: ftp://ftp.openldap.org/incoming/ >>> Submission from: (NULL) (31.130.36.33) >>> >>> >>> 1) LMDB: On 64-bit systems, in some cases mdb_cmp_int() could be >>> called instead >>> of mdb_cmp_long(), when key.mv_size == 8. >>> To reproduce this is enough add an assertion-check for key.size == >>> sizeof(int) >>> into mdb_cmp_int().A%A >> >> You need to provide more details than this. mdb_cmp_int() isn't even >> used on regular databases, only on DUPSORT databases. Provide actual >> code reproducing the issue. >> > this is enough: > - add assert(a->mv_key_size == sizeof(size_t)) into mdb_cmp_long() > - make all test > - see a coredump OK, fixed in git. For the record, this affected MDB_INTEGERDUP, not MDB_INTEGERKEY. >>> 3) lmdb-backend: >>> AttributeDescription->ad_type->sat_equality->smr_indexer() >>> could generate a 5-byte sized ks.s. I can just imagine what this can >>> create a >>> many problems. For instance, when I adds the control for key size, it >>> breaks >>> test017-syncreplication-refresh. But I am not tested more. >> >> Not a bug, indexer keys are padded to 8 bytes. > > There is just NOT padded, I am sure. > Also this is reason for unaligned-access in mdb_cmp_int() and > mdb_cmp_long(). > It is similar to reproduce, just assert + make test. Ah yes, the padding is currently only done on platforms that don't allow unaligned access to ints (e.g. Sparc) git rev e2a7617d17368b5787fc24fbd017623d00fe162b > Seems that bugs together could break ordering of entries, this is > detected by loop over all entries with mdb_cursor_get(..., MDB_NEXT) and > comparing keys from previous step. > All of this I was found today, during developing a mdb_chk tool. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
29.04.2015 19:35, Howard Chu пишет: > Leonid Yuriev wrote: >> this is enough: >> - add assert(a->mv_key_size == sizeof(size_t)) into mdb_cmp_long() >> - make all test >> - see a coredump > > OK, fixed in git. For the record, this affected MDB_INTEGERDUP, not > MDB_INTEGERKEY. > >>>> 3) lmdb-backend: >>>> AttributeDescription->ad_type->sat_equality->smr_indexer() >>>> could generate a 5-byte sized ks.s. I can just imagine what this can >>>> create a >>>> many problems. For instance, when I adds the control for key size, it >>>> breaks >>>> test017-syncreplication-refresh. But I am not tested more. >>> >>> Not a bug, indexer keys are padded to 8 bytes. >> >> There is just NOT padded, I am sure. >> Also this is reason for unaligned-access in mdb_cmp_int() and >> mdb_cmp_long(). >> It is similar to reproduce, just assert + make test. > > Ah yes, the padding is currently only done on platforms that don't > allow unaligned access to ints (e.g. Sparc) > > git rev e2a7617d17368b5787fc24fbd017623d00fe162b > >> Seems that bugs together could break ordering of entries, this is >> detected by loop over all entries with mdb_cursor_get(..., MDB_NEXT) and >> comparing keys from previous step. >> All of this I was found today, during developing a mdb_chk tool. Yes, I was made same local changes few hours ago. This fixs miss-call for mdb_cmp_long(). But what about 5-bytes keys from indexer? - How ones should be compared, as a 4-byte int without MSB, or as a padded 8-byte long? - Where is the padding code? Leonid.
Leonid Yuriev wrote: > 29.04.2015 19:35, Howard Chu пишет: >> Ah yes, the padding is currently only done on platforms that don't >> allow unaligned access to ints (e.g. Sparc) >> >> git rev e2a7617d17368b5787fc24fbd017623d00fe162b > But what about 5-bytes keys from indexer? > - How ones should be compared, as a 4-byte int without MSB, or as a > padded 8-byte long? The indexer doesn't use MDB_INTEGER so your question is irrelevant. > - Where is the padding code? The code is in the git commit ID I already posted in my previous reply. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
changed notes changed state Open to Test moved from Incoming to Software Bugs
29.04.2015 21:55, Howard Chu пишет: > Leonid Yuriev wrote: > >> But what about 5-bytes keys from indexer? >> - How ones should be compared, as a 4-byte int without MSB, or as a >> padded 8-byte long? > > The indexer doesn't use MDB_INTEGER so your question is irrelevant. > OK, but then - what is the purpose of MDB_INTEGERDUP in mdb_attr_dbs_open()? flags = MDB_DUPSORT|MDB_DUPFIXED|MDB_INTEGERDUP; if ( !(slapMode & SLAP_TOOL_READONLY) ) flags |= MDB_CREATE; Think it is not necessary, is it? Leonid.
changed state Test to Release
Leonid Yuriev wrote: > 29.04.2015 21:55, Howard Chu пишет: >> Leonid Yuriev wrote: >> >>> But what about 5-bytes keys from indexer? >>> - How ones should be compared, as a 4-byte int without MSB, or as a >>> padded 8-byte long? >> >> The indexer doesn't use MDB_INTEGER so your question is irrelevant. >> > OK, but then - what is the purpose of MDB_INTEGERDUP in > mdb_attr_dbs_open()? > > flags = MDB_DUPSORT|MDB_DUPFIXED|MDB_INTEGERDUP; > if ( !(slapMode & SLAP_TOOL_READONLY) ) > flags |= MDB_CREATE; > > Think it is not necessary, is it? You really think so? Have you thought about what the actual *values* are in an index, instead of just the keys? Anyway, this ITS is resolved. Thank you for reporting the bug. If you want to ask further questions about how the indexer works take it to openldap-devel. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
test001 still crashes if you insert assert(a->mv_size == sizeof(int) && b->mv_size == sizeof(int)); in mdb_cmp_int(). Using RE24 commit 98f70f1841f85d93 "ITS#8120 Move final...". I don't see "same size" in the INTEGERDUP doc, it's looser than for INTEGERKEY integers. Or have I gone blind again? Anyway, it would be clearer to cut some doc and say data behave like INTEGERKEY keys. Ignoring that, though: Looking yet again at the doc, mdb_dbi_open():INTEGERKEY says "typically" sizeof(int/size_t), I can't see it requires these sizes. Maybe you copied that from a suggestion from me back when I asked for clarifications. "keys must all be of the same size" in the doc is unclear. I gather it should be "... in the DB's lifetime". But it can be read as "...at a given time": Write size_t-sized integers, empty the DB, then write int-sized integers. That does not work, since me_dbxs[].md_dcmp is updated. "mc->mc_dbx->md_dcmp = mdb_cmp_clong;" looks wrong. That updates env->me_dbxs[], I believe. But: If we mdb_txn_abort(), the txn has not been a no-op: It will require next txn and even existing txns to use size_t-sized INTEGERDUP data. The assignment may be non-atomic. Another txn may be reading the pointer. I expect IL32P64 architectures are examples, and also function pointers can be quite large. (OpenLDAP expects void*-sized function pointers though.)
Hallvard Breien Furuseth wrote: > test001 still crashes if you insert > assert(a->mv_size == sizeof(int) && b->mv_size == sizeof(int)); > in mdb_cmp_int(). > Using RE24 commit 98f70f1841f85d93 "ITS#8120 Move final...". Not happening here, all tests pass with asserts enabled. > I don't see "same size" in the INTEGERDUP doc, it's looser > than for INTEGERKEY integers. Or have I gone blind again? > Anyway, it would be clearer to cut some doc and say data > behave like INTEGERKEY keys. Ignoring that, though: > > Looking yet again at the doc, mdb_dbi_open():INTEGERKEY > says "typically" sizeof(int/size_t), I can't see it > requires these sizes. Maybe you copied that from a > suggestion from me back when I asked for clarifications. Probably. The point was to show the expected sizes without enumerating every possible C integer type that would work - e.g. unsigned int, unsigned long, off_t, pid_t, whatever. At some point you have to say "don't be an idiot." > "keys must all be of the same size" in the doc is unclear. > I gather it should be "... in the DB's lifetime". But it > can be read as "...at a given time": Write size_t-sized > integers, empty the DB, then write int-sized integers. > That does not work, since me_dbxs[].md_dcmp is updated. Don't be an idiot. If no time is stated, then it's For All Time. Anything not called out specifically is, by definition, non-specific. > "mc->mc_dbx->md_dcmp = mdb_cmp_clong;" looks wrong. > That updates env->me_dbxs[], I believe. But: > > If we mdb_txn_abort(), the txn has not been a no-op: > It will require next txn and even existing txns to use > size_t-sized INTEGERDUP data. Good point. I've committed a revised patch to leave the mc_dbx pointer alone. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On 03/05/15 09:09, Howard Chu wrote: > Hallvard Breien Furuseth wrote: >> test001 still crashes if you insert >> assert(a->mv_size == sizeof(int) && b->mv_size == sizeof(int)); >> in mdb_cmp_int(). >> Using RE24 commit 98f70f1841f85d93 "ITS#8120 Move final...". > > Not happening here, all tests pass with asserts enabled. You just fixed it. Turns out your older patches to this ITS introduced that one. >> Looking yet again at the doc, mdb_dbi_open():INTEGERKEY >> says "typically" sizeof(int/size_t), I can't see it >> requires these sizes. Maybe you copied that from a >> suggestion from me back when I asked for clarifications. > > Probably. The point was to show the expected sizes without enumerating > every possible C integer type that would work - e.g. unsigned int, > unsigned long, off_t, pid_t, whatever. At some point you have to say > "don't be an idiot." Don't be an idiot. (a) then we'd just say "the architecture's native unsigned integer types" or something, it's no need to be so vague nor to drag in every typedef under the sun. And (b) that's not what the code does anyway. If I understand correctly this time, the doc should simply say that the two types size_t and unsigned int are supported. The "typically" is plain misleading. That is, if mdb_node_search() shows the intent when it picks either int-sized or size_t-sized for IS_BRANCH. OTOH, another bug: default_cmp pick cmp_int for INTEGERDUP|DUPFIXED and some other replaces md_dcmp = cmp_int with cmp_long, which makes no sense unless cmp_int is just a placeholder. Yet at least mdb_dcmp() calls that placeholder directly without checking the data size. And if it's just a placeholder, how about instead setting dcmp to something which just abort()s, to make sure the bug of not replacing it is caught? >> "keys must all be of the same size" in the doc is unclear. >> I gather it should be "... in the DB's lifetime". But it >> can be read as "...at a given time": Write size_t-sized >> integers, empty the DB, then write int-sized integers. >> That does not work, since me_dbxs[].md_dcmp is updated. > > Don't be an idiot. If no time is stated, then it's For All Time. > Anything not called out specifically is, by definition, non-specific. I can't spot the idiocy of expecting a property which springs dynamically into life, to dynamically go away when the data which caused it is gone. In particular since that's how LMDB alignment works. Anyway, that seems to be what happens after latest commit, so maybe it doesn't matter anymore. Maybe the DB flags should include log2(integer sizes) in 2 2-bit fields.
On 04/05/15 02:13, h.b.furuseth@usit.uio.no wrote: > (...) If I understand correctly this time, the doc should > simply say that the two types size_t and unsigned int are supported. > The "typically" is plain misleading. > > That is, if mdb_node_search() shows the intent when it picks either > int-sized or size_t-sized for IS_BRANCH. OTOH, another bug: > > default_cmp pick cmp_int for INTEGERDUP|DUPFIXED and some other > replaces md_dcmp = cmp_int with cmp_long, which makes no sense unless > cmp_int is just a placeholder. Yet at least mdb_dcmp() calls that > placeholder directly without checking the data size. If that's the intent, branch "mdb/keysize" @ my UiO repo fixes it. Should it do the following too? > And if it's > just a placeholder, how about instead setting dcmp to something which > just abort()s, to make sure the bug of not replacing it is caught? Hallvard
changed notes changed state Release to Test
changed state Test to Open
changed notes changed state Open to Test
fix in mdb.master, fixed in mdb.RE/0.9
changed notes changed state Test to Release
changed state Release to Closed