https://bugs.openldap.org/show_bug.cgi?id=9297
Issue ID: 9297 Summary: memory leak in mdb_dn2entry() Product: OpenLDAP Version: unspecified Hardware: All OS: All Status: UNCONFIRMED Severity: normal Priority: --- Component: slapd Assignee: bugs@openldap.org Reporter: grapvar@gmail.com Target Milestone: ---
Callers of mdb_dn2entry() expect that
mdb_dn2entry( ..., Entry **e [out], match = 1 )
provides newborn Entry if and only if [mdb_dn2entry return value in {0, MDB_NOTFOUND}].
They are right with this, because any other error code makes asking for "matching entry" irrelevant.
However, mdb_dn2entry does not anyhow restrict error code when generates a "matching" Entry:
| int | mdb_dn2entry( | ... | struct berval *dn, | Entry **e, | ... | int matched ) | { | ... | int rc = mdb_dn2id( ..., dn, &id, ... ); | if ( rc ) { | if ( matched ) { | int rc2 = mdb_cursor_open( ..., "id2entry", &mc ); | if ( rc2 == MDB_SUCCESS ) { | mdb_id2entry( op, mc, id, e ); | mdb_cursor_close( mc ); | } | } | } else ... | ... | return rc; | }
So, when [mdb_dn2id return value is NOT in {0, MDB_NOTFOUND}], the Entry will be allocated by mdb_id2entry and then leaked by a caller.
Not exactly so. Will be leaked only by mdb_search() and by mdb_add(). It looks like that other callers of mdb_dn2entry() are, by chance, not affected by this issue.
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #1 from Howard Chu hyc@openldap.org --- (In reply to Konstantin Andreev from comment #0)
Callers of mdb_dn2entry() expect that
mdb_dn2entry( ..., Entry **e [out], match = 1 )
provides newborn Entry if and only if [mdb_dn2entry return value in {0, MDB_NOTFOUND}].
They are right with this, because any other error code makes asking for "matching entry" irrelevant.
However, mdb_dn2entry does not anyhow restrict error code when generates a "matching" Entry:
| int | mdb_dn2entry( | ... | struct berval *dn, | Entry **e, | ... | int matched ) | { | ... | int rc = mdb_dn2id( ..., dn, &id, ... ); | if ( rc ) { | if ( matched ) { | int rc2 = mdb_cursor_open( ..., "id2entry", &mc ); | if ( rc2 == MDB_SUCCESS ) { | mdb_id2entry( op, mc, id, e ); | mdb_cursor_close( mc ); | } | } | } else ... | ... | return rc; | }
So, when [mdb_dn2id return value is NOT in {0, MDB_NOTFOUND}], the Entry will be allocated by mdb_id2entry and then leaked by a caller.
What other return codes do you see mdb_dn2id returning?
Not exactly so. Will be leaked only by mdb_search() and by mdb_add(). It looks like that other callers of mdb_dn2entry() are, by chance, not affected by this issue.
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #2 from Konstantin Andreev grapvar@gmail.com --- (In reply to Howard Chu from comment #1)
What other return codes do you see mdb_dn2id returning?
mdb_dn2id() blindly passes mdb_cursor_get() return value to a caller.
I discovered the defect during investigation of interaction in this call chain:
mdb_add <-> mdb_dn2entry <-> mdb_dn2id <-> ...,
so, if you are asking for a test case that triggers leak, I did not care to build it.
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #3 from Howard Chu hyc@openldap.org --- (In reply to Konstantin Andreev from comment #2)
(In reply to Howard Chu from comment #1)
What other return codes do you see mdb_dn2id returning?
mdb_dn2id() blindly passes mdb_cursor_get() return value to a caller.
So again I repeat myself - what other return codes do you see?
I discovered the defect during investigation of interaction in this call chain:
mdb_add <-> mdb_dn2entry <-> mdb_dn2id <-> ...,
so, if you are asking for a test case that triggers leak, I did not care to build it.
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #4 from Konstantin Andreev grapvar@gmail.com --- (In reply to Howard Chu from comment #3)
So again I repeat myself - what other return codes do you see?
What you are asking for is a proof that defect hits the leak.
So I should investigate LMDB sources, develop the test that causes mdb_cursor_get() to return something other than 0 or MDB_NOTFOUND, and, finally, to trigger a leak in mdb_add/mdb_search <-> mdb_dn2entry interaction.
Given that you have officially documented that mdb_cursor_get() returns undocumented error conditions.
To convince you to add plain [rc == MDB_NOTFOUND] check into mdb_dn2entry().
Certainly, I won't do this. Too much work for too low outcome.
https://bugs.openldap.org/show_bug.cgi?id=9297
Howard Chu hyc@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED
--- Comment #5 from Howard Chu hyc@openldap.org --- (In reply to Konstantin Andreev from comment #4)
(In reply to Howard Chu from comment #3)
So again I repeat myself - what other return codes do you see?
What you are asking for is a proof that defect hits the leak.
So I should investigate LMDB sources, develop the test that causes mdb_cursor_get() to return something other than 0 or MDB_NOTFOUND, and, finally, to trigger a leak in mdb_add/mdb_search <-> mdb_dn2entry interaction.
Given that you have officially documented that mdb_cursor_get() returns undocumented error conditions.
cursor_get may return EINVAL when given invalid parameters or for other misuses of its API. back-mdb never misuses the API. Therefore, your concern here is invalid.
Closing this.
https://bugs.openldap.org/show_bug.cgi?id=9297
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |VERIFIED
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #6 from Konstantin Andreev grapvar@gmail.com --- (In reply to Howard Chu from comment #5)
cursor_get may return EINVAL when given invalid parameters or for other misuses of its API. back-mdb never misuses the API. Therefore, your concern here is invalid.
Are I/O errors impossible during mdb_cursor_get()?
https://bugs.openldap.org/show_bug.cgi?id=9297
--- Comment #7 from Howard Chu hyc@openldap.org --- (In reply to Konstantin Andreev from comment #6)
(In reply to Howard Chu from comment #5)
cursor_get may return EINVAL when given invalid parameters or for other misuses of its API. back-mdb never misuses the API. Therefore, your concern here is invalid.
Are I/O errors impossible during mdb_cursor_get()?
By design, yes. There are no syscalls in any read paths in LMDB code, so there are no functions that can return any I/O error status result. Any actual I/O errors occurring during a pagefault would cause a signal instead, not a return code.