IMPALA-5327: Handle return of JNI GetStringUTFChar GetStringUTFChars may return NULL or throw exception and it is not handled in two places. This patch checks the return value/exception, logs if there is error and return error to the caller. A helper class JniUtfCharGuard is added to jni-util.h/cc to manage the lifetime.
Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322 Reviewed-on: http://gerrit.cloudera.org:8080/7642 Reviewed-by: Tim Armstrong <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/79818f02 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/79818f02 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/79818f02 Branch: refs/heads/master Commit: 79818f024f7c153c2b0348c76f1dffce0d59b655 Parents: 51ec607 Author: Tianyi Wang <[email protected]> Authored: Thu Aug 10 16:15:26 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Aug 17 02:29:22 2017 +0000 ---------------------------------------------------------------------- be/src/common/status.h | 5 ++++ be/src/util/jni-util.cc | 48 ++++++++++++++++++++++--------------- be/src/util/jni-util.h | 26 ++++++++++++++++++++ be/src/util/logging-support.cc | 16 +++++++------ 4 files changed, 69 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/79818f02/be/src/common/status.h ---------------------------------------------------------------------- diff --git a/be/src/common/status.h b/be/src/common/status.h index a48470b..91ad907 100644 --- a/be/src/common/status.h +++ b/be/src/common/status.h @@ -269,6 +269,11 @@ std::ostream& operator<<(std::ostream& os, const Status& status); if (UNLIKELY(!__status__.ok())) return __status__; \ } while (false) +#define RETURN_VOID_IF_ERROR(stmt) \ + do { \ + if (UNLIKELY(!(stmt).ok())) return; \ + } while (false) + #define ABORT_IF_ERROR(stmt) \ do { \ Status __status__ = (stmt); \ http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/79818f02/be/src/util/jni-util.cc ---------------------------------------------------------------------- diff --git a/be/src/util/jni-util.cc b/be/src/util/jni-util.cc index 1631ea1..78f67a7 100644 --- a/be/src/util/jni-util.cc +++ b/be/src/util/jni-util.cc @@ -27,6 +27,23 @@ namespace impala { +Status JniUtfCharGuard::create(JNIEnv *env, jstring jstr, JniUtfCharGuard *out) { + DCHECK(!env->ExceptionCheck()); + const char* utf_chars = env->GetStringUTFChars(jstr, nullptr); + bool exception_check = static_cast<bool>(env->ExceptionCheck()); + if (utf_chars == nullptr || exception_check) { + if (exception_check) env->ExceptionClear(); + if (utf_chars != nullptr) env->ReleaseStringUTFChars(jstr, utf_chars); + auto fail_message = "GetStringUTFChars failed. Probable OOM on JVM side"; + LOG(ERROR) << fail_message; + return Status(fail_message); + } + out->env = env; + out->jstr = jstr; + out->utf_chars = utf_chars; + return Status::OK(); +} + jclass JniUtil::jni_util_cl_ = NULL; jclass JniUtil::internal_exc_cl_ = NULL; jmethodID JniUtil::get_jvm_metrics_id_ = NULL; @@ -170,29 +187,23 @@ void JniUtil::InitLibhdfs() { Status JniUtil::GetJniExceptionMsg(JNIEnv* env, bool log_stack, const string& prefix) { jthrowable exc = (env)->ExceptionOccurred(); - if (exc == NULL) return Status::OK(); + if (exc == nullptr) return Status::OK(); env->ExceptionClear(); - DCHECK(throwable_to_string_id() != NULL); - jstring msg = (jstring) env->CallStaticObjectMethod(jni_util_class(), - throwable_to_string_id(), exc); - jboolean is_copy; - string error_msg = - (reinterpret_cast<const char*>(env->GetStringUTFChars(msg, &is_copy))); - + DCHECK(throwable_to_string_id() != nullptr); + auto msg = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(), + throwable_to_string_id(), exc)); + JniUtfCharGuard msg_str_guard; + RETURN_IF_ERROR(JniUtfCharGuard::create(env, msg, &msg_str_guard)); if (log_stack) { - jstring stack = (jstring) env->CallStaticObjectMethod(jni_util_class(), - throwable_to_stack_trace_id(), exc); - const char* c_stack = - reinterpret_cast<const char*>(env->GetStringUTFChars(stack, &is_copy)); - VLOG(1) << string(c_stack); + auto stack = static_cast<jstring>(env->CallStaticObjectMethod(jni_util_class(), + throwable_to_stack_trace_id(), exc)); + JniUtfCharGuard c_stack_guard; + RETURN_IF_ERROR(JniUtfCharGuard::create(env, stack, &c_stack_guard)); + VLOG(1) << c_stack_guard.get(); } - env->ExceptionClear(); env->DeleteLocalRef(exc); - - stringstream ss; - ss << prefix << error_msg; - return Status(ss.str()); + return Status(Substitute("$0$1", prefix, msg_str_guard.get())); } Status JniUtil::GetJvmMetrics(const TGetJvmMetricsRequest& request, @@ -220,5 +231,4 @@ Status JniUtil::LoadStaticJniMethod(JNIEnv* env, const jclass& jni_class, RETURN_ERROR_IF_EXC(env); return Status::OK(); } - } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/79818f02/be/src/util/jni-util.h ---------------------------------------------------------------------- diff --git a/be/src/util/jni-util.h b/be/src/util/jni-util.h index cef10bc..9a9cb15 100644 --- a/be/src/util/jni-util.h +++ b/be/src/util/jni-util.h @@ -142,6 +142,32 @@ struct JniMethodDescriptor { jmethodID* method_id; }; +/// Helper class for lifetime management of chars from JNI, releasing JNI chars when +/// destructed +class JniUtfCharGuard { + public: + /// Construct a JniUtfCharGuards holding nothing + JniUtfCharGuard() : utf_chars(nullptr) {} + + /// Release the held char sequence if there is one. + ~JniUtfCharGuard() { + if (utf_chars != nullptr) env->ReleaseStringUTFChars(jstr, utf_chars); + } + + /// Try to get chars from jstring. If error is returned, utf_chars and get() remain + /// to be nullptr, otherwise they point to a valid char sequence. The char sequence + /// lives as long as this guard. + static Status create(JNIEnv* env, jstring jstr, JniUtfCharGuard* out); + + /// Get the char sequence. Returns nullptr if the guard does hold a char sequence. + const char* get() { return utf_chars; } + private: + JNIEnv* env; + jstring jstr; + const char* utf_chars; + DISALLOW_COPY_AND_ASSIGN(JniUtfCharGuard); +}; + /// Utility class for JNI-related functionality. /// Init() should be called as soon as the native library is loaded. /// Creates global class references, and promotes local references to global references. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/79818f02/be/src/util/logging-support.cc ---------------------------------------------------------------------- diff --git a/be/src/util/logging-support.cc b/be/src/util/logging-support.cc index fd6b8af..3d90ed6 100644 --- a/be/src/util/logging-support.cc +++ b/be/src/util/logging-support.cc @@ -42,11 +42,15 @@ JNIEXPORT void JNICALL Java_org_apache_impala_util_NativeLogger_Log( JNIEnv* env, jclass caller_class, int severity, jstring msg, jstring file, int line_number) { - // Unused required argument to GetStringUTFChars - jboolean dummy; - const char* filename = env->GetStringUTFChars(file, &dummy); + JniUtfCharGuard filename_guard; + RETURN_VOID_IF_ERROR(JniUtfCharGuard::create(env, file, &filename_guard)); + JniUtfCharGuard msg_guard; const char* str = ""; - if (msg != NULL) str = env->GetStringUTFChars(msg, &dummy); + if (msg != nullptr) { + RETURN_VOID_IF_ERROR(JniUtfCharGuard::create(env, msg, &msg_guard)); + str = msg_guard.get(); + } + int log_level = google::INFO; switch (severity) { case TLogLevel::VLOG: @@ -69,9 +73,7 @@ Java_org_apache_impala_util_NativeLogger_Log( default: DCHECK(false) << "Unrecognised TLogLevel: " << log_level; } - google::LogMessage(filename, line_number, log_level).stream() << string(str); - if (msg != NULL) env->ReleaseStringUTFChars(msg, str); - env->ReleaseStringUTFChars(file, filename); + google::LogMessage(filename_guard.get(), line_number, log_level).stream() << str; } namespace {
