Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1070#discussion_r158165539
--- Diff:
exec/memory/base/src/main/java/org/apache/drill/exec/memory/BoundsChecking.java
---
@@ -17,19 +17,92 @@
*/
package org.apache.drill.exec.memory;
+import java.lang.reflect.Field;
+import java.util.Formatter;
+
+import com.google.common.base.Preconditions;
+
+import io.netty.buffer.AbstractByteBuf;
+import io.netty.buffer.DrillBuf;
+import io.netty.util.IllegalReferenceCountException;
+
+import static org.apache.drill.exec.util.SystemPropertyUtil.getBoolean;
+
public class BoundsChecking {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(BoundsChecking.class);
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(BoundsChecking.class);
- public static final boolean BOUNDS_CHECKING_ENABLED;
+ public static final String ENABLE_UNSAFE_BOUNDS_CHECK_PROPERTY =
"drill.exec.memory.enable_unsafe_bounds_check";
+ // for backward compatibility check "drill.enable_unsafe_memory_access"
property and enable bounds checking when
+ // unsafe memory access is explicitly disabled
+ public static final String ENABLE_UNSAFE_MEMORY_ACCESS_PROPERTY =
"drill.enable_unsafe_memory_access";
+ public static final boolean BOUNDS_CHECKING_ENABLED =
+ getBoolean(ENABLE_UNSAFE_BOUNDS_CHECK_PROPERTY,
!getBoolean(ENABLE_UNSAFE_MEMORY_ACCESS_PROPERTY, true));
+ private static final boolean checkAccessible =
getStaticBooleanField(AbstractByteBuf.class, "checkAccessible", false);
static {
- boolean isAssertEnabled = false;
- assert isAssertEnabled = true;
- BOUNDS_CHECKING_ENABLED = isAssertEnabled
- ||
!"true".equals(System.getProperty("drill.enable_unsafe_memory_access"));
+ if (BOUNDS_CHECKING_ENABLED) {
+ logger.warn("Drill is running with direct memory bounds checking
enabled. If this is a production system, disable it.");
+ } else if (logger.isDebugEnabled()) {
+ logger.debug("Direct memory bounds checking is disabled.");
+ }
}
private BoundsChecking() {
}
+ private static boolean getStaticBooleanField(Class cls, String name,
boolean def) {
+ try {
+ Field field = cls.getDeclaredField(name);
+ field.setAccessible(true);
+ return field.getBoolean(null);
+ } catch (ReflectiveOperationException e) {
+ return def;
+ }
+ }
+
+ private static void checkIndex(DrillBuf buf, int index, int fieldLength)
{
+ Preconditions.checkNotNull(buf);
+ if (checkAccessible && buf.refCnt() == 0) {
+ Formatter formatter = new Formatter().format("%s, refCnt: 0", buf);
+ if (BaseAllocator.DEBUG) {
+ formatter.format("%n%s", buf.toVerboseString());
+ }
+ throw new IllegalReferenceCountException(formatter.toString());
+ }
+ if (fieldLength < 0) {
+ throw new IllegalArgumentException(String.format("length: %d
(expected: >= 0)", fieldLength));
+ }
+ if (index < 0 || index > buf.capacity() - fieldLength) {
+ Formatter formatter = new Formatter().format("%s, index: %d, length:
%d (expected: range(0, %d))", buf, index, fieldLength, buf.capacity());
+ if (BaseAllocator.DEBUG) {
+ formatter.format("%n%s", buf.toVerboseString());
+ }
+ throw new IndexOutOfBoundsException(formatter.toString());
+ }
+ }
+
+ public static void lengthCheck(DrillBuf buf, int start, int length) {
+ if (BOUNDS_CHECKING_ENABLED) {
+ checkIndex(buf, start, length);
+ }
+ }
+
+ public static void rangeCheck(DrillBuf buf, int start, int end) {
+ if (BOUNDS_CHECKING_ENABLED) {
+ checkIndex(buf, start, end - start);
+ }
+ }
+
+ public static void rangeCheck(DrillBuf buf1, int start1, int end1,
DrillBuf buf2, int start2, int end2) {
--- End diff --
Why do we need a two-buffer version? Why not two calls to the one-buffer
version for simplicity?
---
