From a280af2a5767b9257c4b4ec44eb27b8c0ff843ca Mon Sep 17 00:00:00 2001 From: fyrz Date: Sat, 15 Nov 2014 12:37:51 +0100 Subject: [PATCH] [RocksJava] Sigsegv fix for MergerOperatorByName --- java/Makefile | 4 +- java/org/rocksdb/ColumnFamilyOptions.java | 2 + java/org/rocksdb/Options.java | 2 + java/org/rocksdb/test/MergeTest.java | 47 +++++++++++++++++++++++ java/rocksjni.pom | 2 +- java/rocksjni/options.cc | 20 ++++++---- 6 files changed, 67 insertions(+), 10 deletions(-) diff --git a/java/Makefile b/java/Makefile index 7a0edb3cf..e2ec5e06d 100644 --- a/java/Makefile +++ b/java/Makefile @@ -56,6 +56,7 @@ JAVA_TESTS = org.rocksdb.test.BackupableDBOptionsTest\ org.rocksdb.test.ComparatorTest\ org.rocksdb.test.DBOptionsTest\ org.rocksdb.test.DirectComparatorTest\ + org.rocksdb.test.EnvironmentTest\ org.rocksdb.test.FilterTest\ org.rocksdb.test.FlushTest\ org.rocksdb.test.InfoLogLevelTest\ @@ -70,6 +71,7 @@ JAVA_TESTS = org.rocksdb.test.BackupableDBOptionsTest\ org.rocksdb.test.RocksDBTest\ org.rocksdb.test.RocksEnvTest\ org.rocksdb.test.RocksIteratorTest\ + org.rocksdb.test.SizeUnitTest\ org.rocksdb.test.SnapshotTest\ org.rocksdb.test.StatisticsCollectorTest\ org.rocksdb.test.WriteBatchHandlerTest\ @@ -130,7 +132,7 @@ resolve_test_deps: test -s "$(JAVA_ASSERTJ_JAR)" || curl -k -L -o "$(JAVA_ASSERTJ_JAR)" http://central.maven.org/maven2/org/assertj/assertj-core/1.7.0/assertj-core-1.7.0.jar test: java resolve_test_deps - java -ea -Djava.library.path=.:../ -cp "$(JAVA_TESTCLASSPATH)" org.rocksdb.test.RocksJunitRunner $(JAVA_TESTS) + java -ea -Xcheck:jni -Djava.library.path=.:../ -cp "$(JAVA_TESTCLASSPATH)" org.rocksdb.test.RocksJunitRunner $(JAVA_TESTS) db_bench: java javac org/rocksdb/benchmark/*.java diff --git a/java/org/rocksdb/ColumnFamilyOptions.java b/java/org/rocksdb/ColumnFamilyOptions.java index 3d3b236a2..86e42bf7d 100644 --- a/java/org/rocksdb/ColumnFamilyOptions.java +++ b/java/org/rocksdb/ColumnFamilyOptions.java @@ -127,6 +127,8 @@ public class ColumnFamilyOptions extends RocksObject @Override public ColumnFamilyOptions setMergeOperatorName(String name) { + assert (isInitialized()); + assert (name != null); setMergeOperatorName(nativeHandle_, name); return this; } diff --git a/java/org/rocksdb/Options.java b/java/org/rocksdb/Options.java index 55f3defd2..68a11e633 100644 --- a/java/org/rocksdb/Options.java +++ b/java/org/rocksdb/Options.java @@ -165,6 +165,8 @@ public class Options extends RocksObject @Override public Options setMergeOperatorName(String name) { + assert (isInitialized()); + assert (name != null); setMergeOperatorName(nativeHandle_, name); return this; } diff --git a/java/org/rocksdb/test/MergeTest.java b/java/org/rocksdb/test/MergeTest.java index 3ebd55975..3dacb3923 100644 --- a/java/org/rocksdb/test/MergeTest.java +++ b/java/org/rocksdb/test/MergeTest.java @@ -8,6 +8,7 @@ package org.rocksdb.test; import java.util.List; import java.util.ArrayList; +import org.junit.Assert; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -252,4 +253,50 @@ public class MergeTest { } } } + + @Test + public void emptyStringInSetMergeOperatorByName() { + Options opt = null; + ColumnFamilyOptions cOpt = null; + try { + opt = new Options(); + cOpt = new ColumnFamilyOptions(); + opt.setMergeOperatorName(""); + cOpt.setMergeOperatorName(""); + } finally { + if (opt != null) { + opt.dispose(); + } + if (cOpt != null) { + cOpt.dispose(); + } + } + } + + @Test(expected = AssertionError.class) + public void nullStringInSetMergeOperatorByNameOptions() { + Options opt = null; + try { + opt = new Options(); + opt.setMergeOperatorName(null); + } finally { + if (opt != null) { + opt.dispose(); + } + } + } + + @Test(expected = AssertionError.class) + public void + nullStringInSetMergeOperatorByNameColumnFamilyOptions() { + ColumnFamilyOptions opt = null; + try { + opt = new ColumnFamilyOptions(); + opt.setMergeOperatorName(null); + } finally { + if (opt != null) { + opt.dispose(); + } + } + } } diff --git a/java/rocksjni.pom b/java/rocksjni.pom index e18a7734d..69e124c48 100644 --- a/java/rocksjni.pom +++ b/java/rocksjni.pom @@ -118,7 +118,7 @@ maven-surefire-plugin 2.17 - ${argLine} + ${argLine} -Xcheck:jni diff --git a/java/rocksjni/options.cc b/java/rocksjni/options.cc index 82fb1fd1b..339a3b095 100644 --- a/java/rocksjni/options.cc +++ b/java/rocksjni/options.cc @@ -146,10 +146,12 @@ void Java_org_rocksdb_Options_setComparatorHandle__JJ( * Signature: (JJjava/lang/String)V */ void Java_org_rocksdb_Options_setMergeOperatorName( - JNIEnv* env, jobject jobj, jlong jhandle, jstring name) { - const char* op_name = env->GetStringUTFChars(name, 0); - reinterpret_cast(jhandle)->merge_operator = - rocksdb::MergeOperators::CreateFromStringId(op_name); + JNIEnv* env, jobject jobj, jlong jhandle, jstring jop_name) { + auto options = reinterpret_cast(jhandle); + const char* op_name = env->GetStringUTFChars(jop_name, 0); + options->merge_operator = rocksdb::MergeOperators::CreateFromStringId( + op_name); + env->ReleaseStringUTFChars(jop_name, op_name); } /* @@ -1884,10 +1886,12 @@ void Java_org_rocksdb_ColumnFamilyOptions_setComparatorHandle__JI( * Signature: (JJjava/lang/String)V */ void Java_org_rocksdb_ColumnFamilyOptions_setMergeOperatorName( - JNIEnv* env, jobject jobj, jlong jhandle, jstring name) { - const char* op_name = env->GetStringUTFChars(name, 0); - reinterpret_cast(jhandle)->merge_operator = - rocksdb::MergeOperators::CreateFromStringId(op_name); + JNIEnv* env, jobject jobj, jlong jhandle, jstring jop_name) { + auto options = reinterpret_cast(jhandle); + const char* op_name = env->GetStringUTFChars(jop_name, 0); + options->merge_operator = rocksdb::MergeOperators::CreateFromStringId( + op_name); + env->ReleaseStringUTFChars(jop_name, op_name); } /*