Full_Name: Hallvard B Furuseth Version: HEAD, RE23 OS: URL: Submission from: (NULL) (129.240.202.105) Submitted by: hallvard overlays/syncprov.c:syncprov_findcsn() sets an unchecked limit to 1. findcsn_cb() says /* We just want to know that at least one exists, so it's OK if * we exceed the unchecked limit or size limit. */ This looks like it can return a false positive if two or more other entries which the filter would eliminate have the same hash as the value syncprov searches for. Also syncprov_findcsn() passes fc_limits uninitialized outside of the .lms_s_unchecked field to slapd. Valgrind complains in test018 about .lms_s_pr_hide in back-bdb/search.c:bdb_search(). Tested in HEAD.
moved from Incoming to Software Bugs
h.b.furuseth@usit.uio.no wrote: > Full_Name: Hallvard B Furuseth > Version: HEAD, RE23 > OS: > URL: > Submission from: (NULL) (129.240.202.105) > Submitted by: hallvard > > > overlays/syncprov.c:syncprov_findcsn() sets an unchecked limit to 1. > findcsn_cb() says > /* We just want to know that at least one exists, so it's OK if > * we exceed the unchecked limit or size limit. > */ > > This looks like it can return a false positive if two or more other > entries which the filter would eliminate have the same hash as the > value syncprov searches for. I don't believe this can cause any problem though. CSN indexing doesn't use a hash the way other indices do; the CSN timestamp is converted to binary form and saved as a 40 bit integer. Index collisions will only occur for multiple changes that occurred within the same 1-second interval. The purpose of this search is to detect if a consumer's context CSN is stale, i.e., the provider no longer has any entries as old as the consumer's CSN. The fact that there is any index collision essentially proves that the CSN is not stale. > Also syncprov_findcsn() passes fc_limits uninitialized outside of the > .lms_s_unchecked field to slapd. Valgrind complains in test018 about > .lms_s_pr_hide in back-bdb/search.c:bdb_search(). Tested in HEAD. Fixed in HEAD. -- -- Howard Chu Chief Architect, 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 Release
changed state Release to Closed
moved from Software Bugs to Archive.Software Bugs
changed notes changed state Closed to Open moved from Archive.Software Bugs to Software Bugs
Reopening this ITS... Howard Chu writes: >> Full_Name: Hallvard B Furuseth >> >> overlays/syncprov.c:syncprov_findcsn() sets an unchecked limit to 1. >> findcsn_cb() says >> /* We just want to know that at least one exists, so it's OK if >> * we exceed the unchecked limit or size limit. >> */ >> >> This looks like it can return a false positive if two or more other >> entries which the filter would eliminate have the same hash as the >> value syncprov searches for. > > I don't believe this can cause any problem though. CSN indexing doesn't use a > hash the way other indices do; the CSN timestamp is converted to binary form > and saved as a 40 bit integer. Index collisions will only occur for multiple > changes that occurred within the same 1-second interval. Only if entryCSN is indexed, which is recommended but not required in man slapo-syncprov. With un-indexed entryCSN it'll hit the unchecked limit if there are two or more entries in scope for the search. Also - another marginal case - findcsn_cb() assumes adminLimitExceeded implies a size limit (.size or .unchecked). It could also mean a hard time limit. After someone did ^Z on slapd while stepping through some debugging, if nothing else. -- Hallvard
Hallvard B Furuseth wrote: > Reopening this ITS... > > Howard Chu writes: >>> Full_Name: Hallvard B Furuseth >>> >>> overlays/syncprov.c:syncprov_findcsn() sets an unchecked limit to 1. >>> findcsn_cb() says >>> /* We just want to know that at least one exists, so it's OK if >>> * we exceed the unchecked limit or size limit. >>> */ >>> >>> This looks like it can return a false positive if two or more other >>> entries which the filter would eliminate have the same hash as the >>> value syncprov searches for. >> >> I don't believe this can cause any problem though. CSN indexing doesn't use a >> hash the way other indices do; the CSN timestamp is converted to binary form >> and saved as a 40 bit integer. Index collisions will only occur for multiple >> changes that occurred within the same 1-second interval. > > Only if entryCSN is indexed, which is recommended but not required in > man slapo-syncprov. With un-indexed entryCSN it'll hit the unchecked > limit if there are two or more entries in scope for the search. > > Also - another marginal case - findcsn_cb() assumes adminLimitExceeded > implies a size limit (.size or .unchecked). It could also mean a hard > time limit. After someone did ^Z on slapd while stepping through some > debugging, if nothing else. Wrong. AdminLimitExceeded ONLY means unchecked here. Otherwise it would be SizeLimitExceeded (or TimeLimitExceeded). -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
hyc@symas.com writes: > Wrong. AdminLimitExceeded ONLY means unchecked here. Otherwise it would be > SizeLimitExceeded (or TimeLimitExceeded). Okay. Took me some digging to see that. Anyway, the main problem is that hitting the unchecked limit does not imply there is an entry which even approximately matches the filter. Not without indexes. -- Hallvard
Hallvard B Furuseth wrote: > hyc@symas.com writes: >> Wrong. AdminLimitExceeded ONLY means unchecked here. Otherwise it would be >> SizeLimitExceeded (or TimeLimitExceeded). > > Okay. Took me some digging to see that. Anyway, the main problem is > that hitting the unchecked limit does not imply there is an entry which > even approximately matches the filter. Not without indexes. > I guess that's a fair point, but operating without an entryCSN index would be a config mistake anyway. (Note that the unchecked limit only has relevance on backends that support indexing in the first place.) -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
hyc@symas.com writes: > I guess that's a fair point, but operating without an entryCSN index would be > a config mistake anyway. (Note that the unchecked limit only has relevance on > backends that support indexing in the first place.) As a user that's a config mistake from which I'd expect poor performance or sizelimit warnings in syslog, not broken CSN handling. In particular when slapo-syncprov(5) does mention indexing without demanding it: "On databases that support inequality indexing, it is helpful to set an eq index on the entryCSN attribute when using this overlay." -- Hallvard
BTW, why do CSN times have 6-decimal precision when we only care about 1/256 second? Or doesn't it matter if this test is gives false positives for small time differences? -- Hallvard
h.b.furuseth@usit.uio.no wrote: > BTW, why do CSN times have 6-decimal precision when we only care > about 1/256 second? Or doesn't it matter if this test is gives > false positives for small time differences? > Yeah, we really just don't care. 6-decimal precision is for conflict resolution, this is just to determine if something is "new enough" where "enough" doesn't need to be exact. -- -- 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 notes changed state Open to Test
changed notes changed state Test to Release
fixed in master fixed in RE24 (2.4.46)
changed notes changed state Release to Closed