From 786dbdc6e4c03a45f4e0c37b8a81a9c08b1f8f05 Mon Sep 17 00:00:00 2001
From: Pixelflinger <pixelflinger@gmail.com>
Date: Wed, 6 May 2020 16:51:19 -0700
Subject: [PATCH] fix JNI objects allocation and memory corruption

- we were allocating objects with a destructor in the command stream
  which is always invalid because there is no guarantee that when
  the callback is called, the underlaying memory is still valid
  (and it wasn't).

- AutoBuffer move-ctor wasn't moving some of its state, which would
  lead to destroying the same ref several times.
---
 android/common/CallbackUtils.cpp                  | 14 +++++++-------
 android/common/CallbackUtils.h                    |  4 ++++
 android/common/NioUtils.cpp                       |  1 +
 android/common/NioUtils.h                         |  2 +-
 android/filament-android/src/main/cpp/Texture.cpp |  5 ++---
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/android/common/CallbackUtils.cpp b/android/common/CallbackUtils.cpp
index 03ea6aaecd..46b9b38344 100644
--- a/android/common/CallbackUtils.cpp
+++ b/android/common/CallbackUtils.cpp
@@ -34,8 +34,7 @@ static void initCallbackJni(JNIEnv* env, CallbackJni& callbackUtils) {
 
 JniBufferCallback* JniBufferCallback::make(filament::Engine* engine,
         JNIEnv* env, jobject handler, jobject callback, AutoBuffer&& buffer) {
-    void* that = engine->streamAlloc(sizeof(JniBufferCallback), alignof(JniBufferCallback));
-    return new (that) JniBufferCallback(env, handler, callback, std::move(buffer));
+    return new JniBufferCallback(env, handler, callback, std::move(buffer));
 }
 
 JniBufferCallback::JniBufferCallback(JNIEnv* env, jobject handler, jobject callback,
@@ -68,14 +67,14 @@ JniBufferCallback::~JniBufferCallback() {
 
 void JniBufferCallback::invoke(void*, size_t, void* user) {
     JniBufferCallback* data = reinterpret_cast<JniBufferCallback*>(user);
-    // don't call delete here, because we don't own the storage
-    data->~JniBufferCallback();
+    delete data;
 }
 
+// -----------------------------------------------------------------------------------------------
+
 JniImageCallback* JniImageCallback::make(filament::Engine* engine,
         JNIEnv* env, jobject handler, jobject callback, long image) {
-    void* that = engine->streamAlloc(sizeof(JniImageCallback), alignof(JniImageCallback));
-    return new (that) JniImageCallback(env, handler, callback, image);
+    return new JniImageCallback(env, handler, callback, image);
 }
 
 JniImageCallback::JniImageCallback(JNIEnv* env, jobject handler, jobject callback, long image)
@@ -106,5 +105,6 @@ JniImageCallback::~JniImageCallback() {
 }
 
 void JniImageCallback::invoke(void*, void* user) {
-    reinterpret_cast<JniImageCallback*>(user)->~JniImageCallback();
+    JniImageCallback* data = reinterpret_cast<JniImageCallback*>(user);
+    delete data;
 }
diff --git a/android/common/CallbackUtils.h b/android/common/CallbackUtils.h
index 4ef710c682..e0c6f47650 100644
--- a/android/common/CallbackUtils.h
+++ b/android/common/CallbackUtils.h
@@ -40,6 +40,8 @@ struct JniBufferCallback {
 
 private:
     JniBufferCallback(JNIEnv* env, jobject handler, jobject callback, AutoBuffer&& buffer);
+    JniBufferCallback(JniBufferCallback const &) = delete;
+    JniBufferCallback(JniBufferCallback&&) = delete;
     ~JniBufferCallback();
 
     JNIEnv* mEnv;
@@ -57,6 +59,8 @@ struct JniImageCallback {
 
 private:
     JniImageCallback(JNIEnv* env, jobject handler, jobject runnable, long image);
+    JniImageCallback(JniBufferCallback const &) = delete;
+    JniImageCallback(JniBufferCallback&&) = delete;
     ~JniImageCallback();
 
     JNIEnv* mEnv;
diff --git a/android/common/NioUtils.cpp b/android/common/NioUtils.cpp
index d060463a2a..3a84997862 100644
--- a/android/common/NioUtils.cpp
+++ b/android/common/NioUtils.cpp
@@ -112,6 +112,7 @@ AutoBuffer::AutoBuffer(AutoBuffer &&rhs) noexcept {
     std::swap(mShift, rhs.mShift);
     std::swap(mBuffer, rhs.mBuffer);
     std::swap(mBaseArray, rhs.mBaseArray);
+    std::swap(mNioUtils, rhs.mNioUtils);
 }
 
 AutoBuffer::~AutoBuffer() noexcept {
diff --git a/android/common/NioUtils.h b/android/common/NioUtils.h
index 2f407cc8fe..94e43d92e3 100644
--- a/android/common/NioUtils.h
+++ b/android/common/NioUtils.h
@@ -72,5 +72,5 @@ private:
         jmethodID getBaseArray;
         jmethodID getBaseArrayOffset;
         jmethodID getBufferType;
-    } mNioUtils;
+    } mNioUtils{};
 };
diff --git a/android/filament-android/src/main/cpp/Texture.cpp b/android/filament-android/src/main/cpp/Texture.cpp
index 8c33a4d170..80b272d552 100644
--- a/android/filament-android/src/main/cpp/Texture.cpp
+++ b/android/filament-android/src/main/cpp/Texture.cpp
@@ -446,12 +446,11 @@ public:
 
     static void invoke(void* buffer, size_t n, void* user) {
         AutoBitmap* data = reinterpret_cast<AutoBitmap*>(user);
-        data->~AutoBitmap();
+        delete data;
     }
 
     static AutoBitmap* make(Engine* engine, JNIEnv* env, jobject bitmap) {
-        void* that = engine->streamAlloc(sizeof(AutoBitmap), alignof(AutoBitmap));
-        return new (that) AutoBitmap(env, bitmap);
+        return new AutoBitmap(env, bitmap);
     }
 
 private: