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 overflow bug in SortingNumericDocValues #70154

Merged
merged 4 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,35 @@ protected final void resize(int newSize) {
count = newSize;
valuesCursor = 0;

if (newSize <= values.length) {
if (newSize <= getArrayLength()) {
return;
}

// Array is expected to grow so increment the circuit breaker
// to include both the additional bytes used by the grown array
// as well as the overhead of keeping both arrays in memory while
// copying.
long oldValuesSizeInBytes = values.length * Long.BYTES;
long oldValuesSizeInBytes = (long) getArrayLength() * Long.BYTES;
int newValuesLength = ArrayUtil.oversize(newSize, Long.BYTES);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the expression above:

long oldValuesSizeInBytes = values.length * Long.BYTES;

Can overflow as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because length and Long.BYTES are both int, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

booooo. And you can't test that one easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that is tricky to test now. we can make getLength a protected method that can be override?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I guess so. I think because this thing is designed for extension maybe we should do something else? These two new methods are only for testing which is fine if you are compositing this thing. But when you extend it its hard to know which to extend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should be just add asserts in the code. I think this is what they are design for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the overrides you can't put the thing into a state where we'd see the overflows. I'd just go with the protected method and a comment, I think. I just kind of got confused.

circuitBreakerConsumer.accept(newValuesLength * Long.BYTES);
circuitBreakerConsumer.accept((long) newValuesLength * Long.BYTES);

// resize
values = ArrayUtil.growExact(values, newValuesLength);
growExact(newValuesLength);

// account for freeing the old values array
circuitBreakerConsumer.accept(-oldValuesSizeInBytes);
}

/** Grow the array in a method so we can override it during testing */
protected void growExact(int newValuesLength) {
values = ArrayUtil.growExact(values, newValuesLength);
}

/** Get the size of the internal array using a method so we can override it during testing */
protected int getArrayLength() {
return values.length;
}

/**
* Sort values that are stored between offsets <code>0</code> and
* {@link #count} of {@link #values}.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.index.fielddata;

import org.apache.lucene.util.ArrayUtil;
import org.elasticsearch.test.ESTestCase;
import org.hamcrest.Matchers;

import java.util.concurrent.atomic.AtomicLong;
import java.util.function.LongConsumer;

public class SortingNumericDocValuesTests extends ESTestCase {

public void testResize() {
final int oldSize = Integer.MAX_VALUE - 200;
final int newSize = Integer.MAX_VALUE - 100;
// This counter should account for the initialization of the array (size == 1)
// and the diff between getArrayLength() and resize()
final AtomicLong counter = new AtomicLong();
LongConsumer consumer = value -> {
long total = counter.addAndGet(value);
assertThat(total, Matchers.greaterThanOrEqualTo(0L));
};
SortingNumericDocValues docValues = new SortingNumericDocValues(consumer) {

@Override
protected void growExact(int newValuesLength) {
// don't grow the array
}

/** Get the size of the internal array using a method so we can override it during testing */
protected int getArrayLength() {
return oldSize;
}

@Override
public boolean advanceExact(int target) {
return false;
}

@Override
public int docID() {
return 0;
}

@Override
public int nextDoc() {
return 0;
}

@Override
public int advance(int target) {
return 0;
}

@Override
public long cost() {
return 0;
}
};
docValues.resize(newSize);
final long internalNewSize = ArrayUtil.oversize(newSize, Long.BYTES);
assertThat(counter.get(), Matchers.lessThan((internalNewSize + 1) * Long.BYTES));
}
}