Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AndDocIdSet#IteratorBasedIterator to not violate initial doc state #5070

Merged
merged 1 commit into from Feb 10, 2014

Conversation

@s1monw
Copy link
Contributor

commented Feb 10, 2014

AndDocIdSet#IteratorBasedIterator was potentially initialized with
NO_MORE_DOCS which violates the initial state of DocIdSetIterator and
could lead to undefined behavior when used in a search context.

Closes #5049

@s1monw s1monw added v1.0.0 labels Feb 10, 2014
@jpountz
jpountz reviewed Feb 10, 2014
View changes
src/main/java/org/elasticsearch/common/lucene/docset/AndDocIdSet.java Outdated
IteratorBasedIterator(DocIdSet[] sets) throws IOException {
iterators = new DocIdSetIterator[sets.length];

public static DocIdSetIterator newDocIdSet(DocIdSet[] sets) throws IOException {

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 10, 2014

Contributor

maybe the method should be named newDocIdSetIterator?

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 10, 2014

Author Contributor

good point

@jpountz
jpountz reviewed Feb 10, 2014
View changes
src/main/java/org/elasticsearch/common/lucene/docset/AndDocIdSet.java Outdated
public static DocIdSetIterator newDocIdSet(DocIdSet[] sets) throws IOException {
if (sets.length == 0) {
return DocIdSetIterator.empty();
}

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 10, 2014

Contributor

should the sets.length == 1 case be optimized as well?

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 10, 2014

Author Contributor

yeah agreeed I will add

@jpountz
jpountz reviewed Feb 10, 2014
View changes
src/main/java/org/elasticsearch/common/lucene/docset/AndDocIdSet.java Outdated
@@ -154,7 +158,9 @@ public final int docID() {
@Override
public final int nextDoc() throws IOException {

if (lastReturn == DocIdSetIterator.NO_MORE_DOCS) return DocIdSetIterator.NO_MORE_DOCS;
if (lastReturn == DocIdSetIterator.NO_MORE_DOCS) {
return DocIdSetIterator.NO_MORE_DOCS;

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 10, 2014

Contributor

Should we have an assert false here? Behavior of nextDoc and advance are undefined when the iterator is exhausted so we shouldn't have anything that relies on the fact that these methods return NO_MORE_DOCS when the iterator is exhausted?

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 10, 2014

Author Contributor

++

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2014

+1 In addition to fixing the bug, I think this makes the iterator creation much better!

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2014

@jpountz pushed a new commit

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2014

+1

AndDocIdSet#IteratorBasedIterator was potentially initialized with
NO_MORE_DOCS which violates the initial state of DocIdSetIterator and
could lead to undefined behavior when used in a search context.

Closes #5049
@s1monw s1monw merged commit 06f8a2e into elastic:master Feb 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.