From 73058d10a8e28fe4e7636ad5c73eeb8651369cdb Mon Sep 17 00:00:00 2001 From: Harsh Shandilya Date: Thu, 5 Mar 2020 21:05:50 +0530 Subject: [PATCH] Resolve various memory leaks (#637) This migrates the clipboard clear logic into a foreground service that allows us to also provide a notification that runs the clear task immediately on click, rather than wait for the timeout. Co-authored-by: Aditya Wasan Signed-off-by: Harsh Shandilya --- CHANGELOG.md | 1 + app/build.gradle | 6 +- app/src/main/AndroidManifest.xml | 4 + .../com/zeapo/pwdstore/ClipboardService.kt | 156 ++++++++++++++++++ .../java/com/zeapo/pwdstore/PasswordStore.kt | 5 + .../pwdstore/autofill/AutofillService.kt | 4 +- .../com/zeapo/pwdstore/crypto/PgpActivity.kt | 125 +++++++------- .../zeapo/pwdstore/utils/ClipboardUtils.kt | 28 ++++ app/src/main/res/layout/decrypt_layout.xml | 10 -- app/src/main/res/values/strings.xml | 2 +- dependencies.gradle | 2 + 11 files changed, 259 insertions(+), 84 deletions(-) create mode 100644 app/src/main/java/com/zeapo/pwdstore/ClipboardService.kt create mode 100644 app/src/main/java/com/zeapo/pwdstore/utils/ClipboardUtils.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b6f4c68..8e63a360 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file. - Swipe on password list to synchronize repository ### Fixed +- Resolve memory leaks on password decryption - Can't delete folders containing a password ## [1.5.0] - 2020-02-21 diff --git a/app/build.gradle b/app/build.gradle index adf9e3f7..fa85c61b 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -80,9 +80,12 @@ dependencies { implementation deps.androidx.annotation implementation deps.androidx.appcompat implementation deps.androidx.biometric - implementation deps.androidx.core_ktx implementation deps.androidx.constraint_layout + implementation deps.androidx.core_ktx implementation deps.androidx.documentfile + implementation deps.androidx.lifecycle_runtime_ktx + implementation deps.androidx.local_broadcast_manager + implementation deps.androidx.material implementation deps.androidx.preference implementation deps.androidx.swiperefreshlayout constraints { @@ -90,7 +93,6 @@ dependencies { because 'versions above 1.0.0 have an accessibility related bug that causes crashes' } } - implementation deps.androidx.material implementation deps.kotlin.coroutines.android implementation deps.kotlin.coroutines.core diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 9f1b8081..75f0752a 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -14,6 +14,7 @@ + + { + clearClipboard() + stopForeground(true) + stopSelf() + return super.onStartCommand(intent, flags, startId) + } + + ACTION_START -> { + val time = try { + Integer.parseInt(settings.getString("general_show_time", "45") as String) + } catch (e: NumberFormatException) { + 45 + } + + if (time == 0) { + stopSelf() + } + + createNotification() + scope.launch { + withContext(Dispatchers.IO) { + startTimer(time) + } + withContext(Dispatchers.Main) { + emitBroadcast() + clearClipboard() + stopForeground(true) + stopSelf() + } + } + return START_NOT_STICKY + } + } + } + + return super.onStartCommand(intent, flags, startId) + } + + override fun onBind(intent: Intent?): IBinder? { + return null + } + + override fun onDestroy() { + scope.cancel() + super.onDestroy() + } + + private fun clearClipboard() { + val deepClear = settings.getBoolean("clear_clipboard_20x", false) + val clipboardManager = getSystemService() + + if (clipboardManager is ClipboardManager) { + scope.launch { + ClipboardUtils.clearClipboard(clipboardManager, deepClear) + } + } else { + Timber.tag("ClipboardService").d("Cannot get clipboard manager service") + } + } + + private suspend fun startTimer(showTime: Int) { + var current = 0 + while (scope.isActive && current < showTime) { + // Block for 1s or until cancel is signalled + current++ + delay(1000) + } + } + + private fun emitBroadcast() { + val localBroadcastManager = LocalBroadcastManager.getInstance(this) + val clearIntent = Intent(ACTION_CLEAR) + localBroadcastManager.sendBroadcast(clearIntent) + } + + private fun createNotification() { + createNotificationChannel() + val clearIntent = Intent(this, ClipboardService::class.java) + clearIntent.action = ACTION_CLEAR + val pendingIntent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + PendingIntent.getForegroundService(this, 0, clearIntent, PendingIntent.FLAG_UPDATE_CURRENT) + } else { + PendingIntent.getService(this, 0, clearIntent, PendingIntent.FLAG_UPDATE_CURRENT) + } + + val notification = NotificationCompat.Builder(this, CHANNEL_ID) + .setContentTitle(getString(R.string.app_name)) + .setContentText(getString(R.string.tap_clear_clipboard)) + .setSmallIcon(R.drawable.ic_launcher_foreground) + .setContentIntent(pendingIntent) + .setUsesChronometer(true) + .setPriority(NotificationCompat.PRIORITY_LOW) + .build() + + startForeground(1, notification) + } + + private fun createNotificationChannel() { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + val serviceChannel = NotificationChannel( + CHANNEL_ID, + getString(R.string.app_name), + NotificationManager.IMPORTANCE_LOW + ) + val manager = getSystemService() + if (manager != null) { + manager.createNotificationChannel(serviceChannel) + } else { + Timber.tag("ClipboardService").d("Failed to create notification channel") + } + } + } + companion object { + private const val ACTION_CLEAR = "ACTION_CLEAR_CLIPBOARD" + private const val ACTION_START = "ACTION_START_CLIPBOARD_TIMER" + private const val CHANNEL_ID = "NotificationService" + } +} diff --git a/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt b/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt index 5cb0316e..f0e5645a 100644 --- a/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt +++ b/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt @@ -254,6 +254,11 @@ class PasswordStore : AppCompatActivity() { return super.onOptionsItemSelected(item) } + override fun onDestroy() { + plist = null + super.onDestroy() + } + fun openSettings(view: View?) { val intent: Intent try { diff --git a/app/src/main/java/com/zeapo/pwdstore/autofill/AutofillService.kt b/app/src/main/java/com/zeapo/pwdstore/autofill/AutofillService.kt index 22e49919..2d9eb813 100644 --- a/app/src/main/java/com/zeapo/pwdstore/autofill/AutofillService.kt +++ b/app/src/main/java/com/zeapo/pwdstore/autofill/AutofillService.kt @@ -518,7 +518,7 @@ class AutofillService : AccessibilityService(), CoroutineScope by CoroutineScope if (entry?.hasUsername() == true) { lastPassword = entry val ttl = Integer.parseInt(settings!!.getString("general_show_time", "45")!!) - Toast.makeText(applicationContext, getString(R.string.autofill_toast_username, ttl), Toast.LENGTH_LONG).show() + withContext(Dispatchers.Main) { Toast.makeText(applicationContext, getString(R.string.autofill_toast_username, ttl), Toast.LENGTH_LONG).show() } lastPasswordMaxDate = System.currentTimeMillis() + ttl * 1000L } } catch (e: UnsupportedEncodingException) { @@ -537,7 +537,7 @@ class AutofillService : AccessibilityService(), CoroutineScope by CoroutineScope OpenPgpApi.RESULT_CODE_ERROR -> { val error = result.getParcelableExtra(OpenPgpApi.RESULT_ERROR) if (error != null) { - Toast.makeText(applicationContext, "Error from OpenKeyChain : ${error.message}", Toast.LENGTH_LONG).show() + withContext(Dispatchers.Main) { Toast.makeText(applicationContext, "Error from OpenKeyChain : ${error.message}", Toast.LENGTH_LONG).show() } Timber.tag(Constants.TAG).e("onError getErrorId: ${error.errorId}") Timber.tag(Constants.TAG).e("onError getMessage: ${error.message}") } diff --git a/app/src/main/java/com/zeapo/pwdstore/crypto/PgpActivity.kt b/app/src/main/java/com/zeapo/pwdstore/crypto/PgpActivity.kt index 42878171..9e4a7130 100644 --- a/app/src/main/java/com/zeapo/pwdstore/crypto/PgpActivity.kt +++ b/app/src/main/java/com/zeapo/pwdstore/crypto/PgpActivity.kt @@ -5,17 +5,17 @@ package com.zeapo.pwdstore.crypto import android.app.PendingIntent +import android.content.BroadcastReceiver import android.content.ClipData import android.content.ClipboardManager import android.content.Context import android.content.Intent +import android.content.IntentFilter import android.content.IntentSender import android.content.SharedPreferences import android.graphics.Typeface -import android.os.AsyncTask +import android.os.Build import android.os.Bundle -import android.os.ConditionVariable -import android.os.Handler import android.text.TextUtils import android.text.format.DateUtils import android.text.method.PasswordTransformationMethod @@ -27,13 +27,15 @@ import android.view.View import android.view.WindowManager import android.widget.Button import android.widget.CheckBox -import android.widget.ProgressBar import android.widget.TextView import androidx.appcompat.app.AppCompatActivity import androidx.constraintlayout.widget.ConstraintLayout +import androidx.lifecycle.lifecycleScope +import androidx.localbroadcastmanager.content.LocalBroadcastManager import androidx.preference.PreferenceManager import com.google.android.material.dialog.MaterialAlertDialogBuilder import com.google.android.material.snackbar.Snackbar +import com.zeapo.pwdstore.ClipboardService import com.zeapo.pwdstore.PasswordEntry import com.zeapo.pwdstore.R import com.zeapo.pwdstore.UserPreference @@ -52,7 +54,6 @@ import kotlinx.android.synthetic.main.encrypt_layout.crypto_password_edit import kotlinx.android.synthetic.main.encrypt_layout.crypto_password_file_edit import kotlinx.android.synthetic.main.encrypt_layout.generate_password import kotlinx.coroutines.Dispatchers.IO -import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch import me.msfjarvis.openpgpktx.util.OpenPgpApi import me.msfjarvis.openpgpktx.util.OpenPgpApi.Companion.ACTION_DECRYPT_VERIFY @@ -96,10 +97,15 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { private val relativeParentPath: String by lazy { getParentPath(fullPath, repoPath) } val settings: SharedPreferences by lazy { PreferenceManager.getDefaultSharedPreferences(this) } - private val keyIDs: MutableSet by lazy { - settings.getStringSet("openpgp_key_ids_set", mutableSetOf()) ?: emptySet() - } + private val keyIDs get() = _keyIDs + private var _keyIDs = emptySet() private var mServiceConnection: OpenPgpServiceConnection? = null + private var delayTask: DelayShow? = null + private val receiver = object : BroadcastReceiver() { + override fun onReceive(context: Context?, intent: Intent?) { + delayTask?.doOnPostExecute() + } + } override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -107,6 +113,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { Timber.tag(TAG) // some persistence + _keyIDs = settings.getStringSet("openpgp_key_ids_set", null) ?: emptySet() val providerPackageName = settings.getString("openpgp_provider_list", "") if (TextUtils.isEmpty(providerPackageName)) { @@ -127,7 +134,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { crypto_password_file.text = name crypto_password_file.setOnLongClickListener { val clip = ClipData.newPlainText("pgp_handler_result_pm", name) - clipboard.setPrimaryClip(clip) + clipboard.primaryClip = clip showSnackbar(this.resources.getString(R.string.clipboard_username_toast_text)) true } @@ -157,6 +164,16 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { } } + override fun onResume() { + super.onResume() + LocalBroadcastManager.getInstance(this).registerReceiver(receiver, IntentFilter(ACTION_CLEAR)) + } + + override fun onStop() { + LocalBroadcastManager.getInstance(this).unregisterReceiver(receiver) + super.onStop() + } + override fun onDestroy() { checkAndIncrementHotp() super.onDestroy() @@ -258,7 +275,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { val iStream = FileUtils.openInputStream(File(fullPath)) val oStream = ByteArrayOutputStream() - GlobalScope.launch(IO) { + lifecycleScope.launch(IO) { api?.executeApiAsync(data, iStream, oStream, object : OpenPgpApi.IOpenPgpCallback { override fun onReturn(result: Intent?) { when (result?.getIntExtra(RESULT_CODE, RESULT_CODE_ERROR)) { @@ -470,7 +487,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { val path = if (intent.getBooleanExtra("fromDecrypt", false)) fullPath else "$fullPath/$editName.gpg" - GlobalScope.launch(IO) { + lifecycleScope.launch(IO) { api?.executeApiAsync(data, iStream, oStream, object : OpenPgpApi.IOpenPgpCallback { override fun onReturn(result: Intent?) { when (result?.getIntExtra(RESULT_CODE, RESULT_CODE_ERROR)) { @@ -582,7 +599,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { private fun getKeyIds(receivedIntent: Intent? = null) { val data = receivedIntent ?: Intent() data.action = OpenPgpApi.ACTION_GET_KEY_IDS - GlobalScope.launch(IO) { + lifecycleScope.launch(IO) { api?.executeApiAsync(data, null, null, object : OpenPgpApi.IOpenPgpCallback { override fun onReturn(result: Intent?) { when (result?.getIntExtra(RESULT_CODE, RESULT_CODE_ERROR)) { @@ -689,7 +706,7 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { } val clip = ClipData.newPlainText("pgp_handler_result_pm", pass) - clipboard.setPrimaryClip(clip) + clipboard.primaryClip = clip var clearAfter = 45 try { @@ -708,13 +725,13 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { private fun copyUsernameToClipBoard(username: String) { val clip = ClipData.newPlainText("pgp_handler_result_pm", username) - clipboard.setPrimaryClip(clip) + clipboard.primaryClip = clip showSnackbar(resources.getString(R.string.clipboard_username_toast_text)) } private fun copyOtpToClipBoard(code: String) { val clip = ClipData.newPlainText("pgp_handler_result_pm", code) - clipboard.setPrimaryClip(clip) + clipboard.primaryClip = clip showSnackbar(resources.getString(R.string.clipboard_otp_toast_text)) } @@ -741,8 +758,8 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { delayTask?.cancelAndSignal(true) // launch a new one - delayTask = DelayShow(this) - delayTask?.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) + delayTask = DelayShow() + delayTask?.execute() } /** @@ -758,11 +775,10 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { } @Suppress("StaticFieldLeak") - inner class DelayShow(val activity: PgpActivity) : AsyncTask() { - private val pb: ProgressBar? by lazy { pbLoading } - private var skip = false - private var cancelNotify = ConditionVariable() + inner class DelayShow { + private var skip = false + private var service: Intent? = null private var showTime: Int = 0 // Custom cancellation that can be triggered from another thread. @@ -772,14 +788,25 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { // is true, the cancelled task won't clear the clipboard. fun cancelAndSignal(skipClearing: Boolean) { skip = skipClearing - cancelNotify.open() + if (service != null) { + stopService(service) + service = null + } } - val settings: SharedPreferences by lazy { - PreferenceManager.getDefaultSharedPreferences(activity) + fun execute() { + service = Intent(this@PgpActivity, ClipboardService::class.java).also { + it.action = ACTION_START + } + doOnPreExecute() + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + startForegroundService(service) + } else { + startService(service) + } } - override fun onPreExecute() { + private fun doOnPreExecute() { showTime = try { Integer.parseInt(settings.getString("general_show_time", "45") as String) } catch (e: NumberFormatException) { @@ -793,49 +820,12 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { if (extraText?.text?.isNotEmpty() == true) findViewById(R.id.crypto_extra_show_layout)?.visibility = View.VISIBLE - - if (showTime == 0) { - // treat 0 as forever, and the user must exit and/or clear clipboard on their own - cancel(true) - } else { - this.pb?.max = showTime - } } - override fun doInBackground(vararg params: Void): Boolean? { - var current = 0 - while (current < showTime) { - - // Block for 1s or until cancel is signalled - if (cancelNotify.block(1000)) { - return true - } - - current++ - publishProgress(current) - } - return true - } - - override fun onPostExecute(b: Boolean?) { + fun doOnPostExecute() { if (skip) return checkAndIncrementHotp() - // No need to validate clear_after_copy. It was validated in copyPasswordToClipBoard() - Timber.d("Clearing the clipboard") - val clip = ClipData.newPlainText("pgp_handler_result_pm", "") - clipboard.setPrimaryClip(clip) - if (settings.getBoolean("clear_clipboard_20x", false)) { - val handler = Handler() - for (i in 0..19) { - val count = i.toString() - handler.postDelayed( - { clipboard.setPrimaryClip(ClipData.newPlainText(count, count)) }, - (i * 500).toLong() - ) - } - } - if (crypto_password_show != null) { // clear password; if decrypt changed to encrypt layout via edit button, no need if (passwordEntry?.hotpIsIncremented() == false) { @@ -849,10 +839,6 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { finish() } } - - override fun onProgressUpdate(vararg values: Int?) { - this.pb?.progress = values[0] ?: 0 - } } companion object { @@ -860,13 +846,14 @@ class PgpActivity : AppCompatActivity(), OpenPgpServiceConnection.OnBound { const val REQUEST_DECRYPT = 202 const val REQUEST_KEY_ID = 203 + private const val ACTION_CLEAR = "ACTION_CLEAR_CLIPBOARD" + private const val ACTION_START = "ACTION_START_CLIPBOARD_TIMER" + const val TAG = "PgpActivity" const val KEY_PWGEN_TYPE_CLASSIC = "classic" const val KEY_PWGEN_TYPE_XKPASSWD = "xkpasswd" - private var delayTask: DelayShow? = null - /** * Gets the relative path to the repository */ diff --git a/app/src/main/java/com/zeapo/pwdstore/utils/ClipboardUtils.kt b/app/src/main/java/com/zeapo/pwdstore/utils/ClipboardUtils.kt new file mode 100644 index 00000000..d173a519 --- /dev/null +++ b/app/src/main/java/com/zeapo/pwdstore/utils/ClipboardUtils.kt @@ -0,0 +1,28 @@ +/* + * Copyright © 2014-2020 The Android Password Store Authors. All Rights Reserved. + * SPDX-License-Identifier: GPL-3.0-only + */ +package com.zeapo.pwdstore.utils + +import android.content.ClipData +import android.content.ClipboardManager +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import timber.log.Timber + +object ClipboardUtils { + + suspend fun clearClipboard(clipboard: ClipboardManager, deepClear: Boolean = false) { + Timber.d("Clearing the clipboard") + val clip = ClipData.newPlainText("pgp_handler_result_pm", "") + clipboard.primaryClip = clip + if (deepClear) { + withContext(Dispatchers.IO) { + repeat(20) { + val count = (it * 500).toString() + clipboard.primaryClip = ClipData.newPlainText(count, count) + } + } + } + } +} diff --git a/app/src/main/res/layout/decrypt_layout.xml b/app/src/main/res/layout/decrypt_layout.xml index 17ab55ed..8f8198a4 100644 --- a/app/src/main/res/layout/decrypt_layout.xml +++ b/app/src/main/res/layout/decrypt_layout.xml @@ -94,16 +94,6 @@ app:layout_constraintBaseline_toBaselineOf="@id/crypto_password_show_label" android:typeface="monospace" /> - - Always search from root Search from root of store regardless of currently open directory Password Generator - + Tap here to clear clipboard diff --git a/dependencies.gradle b/dependencies.gradle index 3647ccbb..7828b109 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -34,6 +34,8 @@ ext.deps = [ constraint_layout: 'androidx.constraintlayout:constraintlayout:2.0.0-beta4', core_ktx: 'androidx.core:core-ktx:1.3.0-alpha01', documentfile: 'androidx.documentfile:documentfile:1.0.1', + lifecycle_runtime_ktx: 'androidx.lifecycle:lifecycle-runtime-ktx:2.2.0-alpha01', + local_broadcast_manager: 'androidx.localbroadcastmanager:localbroadcastmanager:1.0.0', material: 'com.google.android.material:material:1.2.0-alpha05', preference: 'androidx.preference:preference:1.1.0', recycler_view: 'androidx.recyclerview:recyclerview:1.0.0',