Merge "Fix the use-after-free issue with CREATE_UTF_OR_RETURN." into main

2 files changed, 53 insertions, 9 deletions

diff --git a/header_only_include/nativehelper/utils.h b/header_only_include/nativehelper/utils.h
index 30f239e..12f591b 100644
--- a/header_only_include/nativehelper/utils.h
+++ b/header_only_include/nativehelper/utils.h
@@ -35,8 +35,8 @@ namespace jnihelp {
 // Implementation details. DO NOT use directly.
 namespace internal {
 
-[[maybe_unused]] static const char* get_c_str(const char* str) { return str; }
-[[maybe_unused]] static const char* get_c_str(const std::string& str) { return str.c_str(); }
+[[maybe_unused]] static const char* GetCStr(const char* str) { return str; }
+[[maybe_unused]] static const char* GetCStr(const std::string& str) { return str.c_str(); }
 
 }  // namespace internal
 
@@ -103,12 +103,12 @@ class JniDefaultValue {
 
 #define GET_UTF_OR_RETURN_IMPL_(env, expr, ...)                          \
     ({                                                                   \
-        ScopedUtfChars tmp_scoped_utf_chars_(env, expr);                 \
-        if (tmp_scoped_utf_chars_.c_str() == nullptr) {                  \
+        ScopedUtfChars __or_return_scoped_utf_chars(env, expr);          \
+        if (__or_return_scoped_utf_chars.c_str() == nullptr) {           \
             /* Return with a pending exception from `ScopedUtfChars`. */ \
             return __VA_ARGS__;                                          \
         }                                                                \
-        std::move(tmp_scoped_utf_chars_);                                \
+        std::move(__or_return_scoped_utf_chars);                         \
     })
 
 // Creates `ScopedLocalRef<jstring>` from a `const char*` or `std::string` expression using
@@ -139,15 +139,19 @@ class JniDefaultValue {
 
 #define CREATE_UTF_OR_RETURN_IMPL_(env, expr, ...)                                             \
     ({                                                                                         \
-        const char* tmp_c_str_ = android::jnihelp::internal::get_c_str(expr);                  \
-        ScopedLocalRef<jstring> tmp_local_ref_(env, env->NewStringUTF(tmp_c_str_));            \
+        const char* __or_return_c_str;                                                         \
+        ScopedLocalRef<jstring> __or_return_local_ref(                                         \
+            env,                                                                               \
+            env->NewStringUTF(__or_return_c_str = android::jnihelp::internal::GetCStr(expr))); \
+        /* `*__or_return_c_str` may be freed here, but we only compare the pointer against     \
+         * nullptr. DO NOT DEREFERENCE `*__or_return_c_str` after this point. */               \
         /* `NewStringUTF` returns nullptr when OOM or the input is nullptr, but only throws an \
          * exception when OOM. */                                                              \
-        if (tmp_local_ref_ == nullptr && tmp_c_str_ != nullptr) {                              \
+        if (__or_return_local_ref == nullptr && __or_return_c_str != nullptr) {                \
             /* Return with a pending exception from `NewStringUTF`. */                         \
             return __VA_ARGS__;                                                                \
         }                                                                                      \
-        std::move(tmp_local_ref_);                                                             \
+        std::move(__or_return_local_ref);                                                      \
     })
 
 }  // namespace jnihelp
diff --git a/tests_mts/jni/libnativehelper_test.cpp b/tests_mts/jni/libnativehelper_test.cpp
index 1063794..bd448f9 100644
--- a/tests_mts/jni/libnativehelper_test.cpp
+++ b/tests_mts/jni/libnativehelper_test.cpp
@@ -103,6 +103,46 @@ TEST_F(LibnativehelperTest, CreateUtfOrReturn) {
     EXPECT_EQ(ret, 1);
 }
 
+class MyString : public std::string {
+   public:
+    explicit MyString(const char* c_str) : std::string(c_str) {}
+
+    // Force clear the string to catch use-after-free issues.
+    ~MyString() { clear(); }
+};
+
+// `expr` creates a temporary object and evaluates to it.
+TEST_F(LibnativehelperTest, CreateUtfOrReturnExprEvaluatesToTemporary) {
+    std::unique_ptr<ScopedLocalRef<jstring>> result;
+
+    jint ret = [&](JNIEnv* env) -> jint {
+        ScopedLocalRef<jstring> j_str = CREATE_UTF_OR_RETURN(env, MyString("foo"));
+        result.reset(new ScopedLocalRef<jstring>(std::move(j_str)));
+        return 1;
+    }(mEnv);
+
+    ScopedUtfChars str(mEnv, result->get());
+    EXPECT_EQ(str.c_str(), std::string_view("foo"));
+    EXPECT_FALSE(mEnv->ExceptionCheck());
+    EXPECT_EQ(ret, 1);
+}
+
+// `expr` creates a temporary object and evaluates to something else backed by it.
+TEST_F(LibnativehelperTest, CreateUtfOrReturnExprEvaluatesToValueBackedByTemporary) {
+    std::unique_ptr<ScopedLocalRef<jstring>> result;
+
+    jint ret = [&](JNIEnv* env) -> jint {
+        ScopedLocalRef<jstring> j_str = CREATE_UTF_OR_RETURN(env, MyString("foo").c_str());
+        result.reset(new ScopedLocalRef<jstring>(std::move(j_str)));
+        return 1;
+    }(mEnv);
+
+    ScopedUtfChars str(mEnv, result->get());
+    EXPECT_EQ(str.c_str(), std::string_view("foo"));
+    EXPECT_FALSE(mEnv->ExceptionCheck());
+    EXPECT_EQ(ret, 1);
+}
+
 TEST_F(LibnativehelperTest, CreateUtfOrReturnVoid) {
     std::unique_ptr<ScopedLocalRef<jstring>> result;