From 3a5ecc5e8ec6c820dbfdb788dc06f7dbb0699c18 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 25 Apr 2018 17:10:03 +0200 Subject: [PATCH] do not crash when getting hash of APK that disappears This changes the logic of Utils.getBinaryHash() to return null on failure rather than only throwing exceptions. That makes it easier to handle these failures where Utils.getBinaryHash() is called. #1305 #855 --- .../main/java/org/fdroid/fdroid/Utils.java | 3 +- .../main/java/org/fdroid/fdroid/data/Apk.java | 3 +- .../org/fdroid/fdroid/data/ApkProvider.java | 5 +- .../main/java/org/fdroid/fdroid/data/App.java | 47 ++++++++++--------- .../localrepo/CacheSwapAppsService.java | 6 ++- .../fdroid/localrepo/LocalRepoManager.java | 4 +- .../fdroid/fdroid/localrepo/SwapService.java | 2 +- 7 files changed, 40 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index 73944d1fa..0f5c938f4 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -492,6 +492,7 @@ public final class Utils { * probably warranted. See https://www.gitlab.com/fdroid/fdroidclient/issues/855 * for more detail. */ + @Nullable public static String getBinaryHash(File apk, String algo) { FileInputStream fis = null; try { @@ -514,12 +515,12 @@ public final class Utils { } else if (message.contains(" ENOENT ")) { Utils.debugLog(TAG, apk + " vanished: " + message); } - throw new IllegalArgumentException(e); } catch (NoSuchAlgorithmException e) { throw new IllegalArgumentException(e); } finally { closeQuietly(fis); } + return null; } /** diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 0c14c1fd5..2618edd94 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -70,6 +70,7 @@ public class Apk extends ValueObject implements Comparable, Parcelable { public String versionName; public int versionCode; public int size; // Size in bytes - 0 means we don't know! + @NonNull public String hash; // checksum of the APK, in lowercase hex public String hashType; public int minSdkVersion = SDK_VERSION_MIN_VALUE; // 0 if unknown @@ -358,7 +359,7 @@ public class Apk extends ValueObject implements Comparable, Parcelable { @Override @TargetApi(19) - public int compareTo(Apk apk) { + public int compareTo(@NonNull Apk apk) { if (Build.VERSION.SDK_INT < 19) { return Integer.valueOf(versionCode).compareTo(apk.versionCode); } diff --git a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java index 73a8b7da1..3a1ea2cf1 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -9,7 +9,6 @@ import android.net.Uri; import android.support.annotation.NonNull; import android.support.annotation.Nullable; import android.util.Log; - import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Schema.AntiFeatureTable; import org.fdroid.fdroid.data.Schema.ApkAntiFeatureJoinTable; @@ -20,6 +19,7 @@ import org.fdroid.fdroid.data.Schema.PackageTable; import org.fdroid.fdroid.data.Schema.RepoTable; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -164,6 +164,9 @@ public class ApkProvider extends FDroidProvider { @NonNull public static List findApksByHash(Context context, String apkHash) { + if (apkHash == null) { + return Collections.emptyList(); + } ContentResolver resolver = context.getContentResolver(); final Uri uri = getContentUri(); String selection = " apk." + Cols.HASH + " = ? "; diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index c89b92185..33b6b5ee4 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -369,15 +369,33 @@ public class App extends ValueObject implements Comparable, Parcelable { /** * Instantiate from a locally installed package. + *

+ * Initializes an {@link App} instances from an APK file. Since the file + * could in the cache, and files can disappear from the cache at any time, + * this needs to be quite defensive ensuring that {@code apkFile} still + * exists. */ - public App(Context context, PackageManager pm, String packageName) + @Nullable + public static App getInstance(Context context, PackageManager pm, String packageName) throws CertificateEncodingException, IOException, PackageManager.NameNotFoundException { - + App app = new App(); PackageInfo packageInfo = pm.getPackageInfo(packageName, PackageManager.GET_PERMISSIONS); - setFromPackageInfo(pm, packageInfo); - this.installedApk = new Apk(); SanitizedFile apkFile = SanitizedFile.knownSanitized(packageInfo.applicationInfo.publicSourceDir); - initApkFromApkFile(context, this.installedApk, packageInfo, apkFile); + if (apkFile.canRead()) { + String hashType = "SHA-256"; + String hash = Utils.getBinaryHash(apkFile, hashType); + if (TextUtils.isEmpty(hash)) { + return null; + } + app.installedApk.hashType = hashType; + app.installedApk.hash = hash; + app.installedApk.sig = Utils.getPackageSig(packageInfo); + } + + app.setFromPackageInfo(pm, packageInfo); + app.installedApk = new Apk(); + app.initInstalledApk(context, app.installedApk, packageInfo, apkFile); + return app; } /** @@ -704,22 +722,6 @@ public class App extends ValueObject implements Comparable, Parcelable { this.compatible = true; } - /** - * Initializes an {@link App} instances from an APK file. Since the file - * could in the cache, and files can disappear from the cache at any time, - * this needs to be quite defensive ensuring that {@code apkFile} still - * exists. - */ - private void initApkFromApkFile(Context context, Apk apk, PackageInfo packageInfo, SanitizedFile apkFile) - throws IOException, CertificateEncodingException { - if (apkFile.canRead()) { - apk.hashType = "sha256"; - apk.hash = Utils.getBinaryHash(apkFile, apk.hashType); - apk.sig = Utils.getPackageSig(packageInfo); - } - initInstalledApk(context, apk, packageInfo, apkFile); - } - public static void initInstalledObbFiles(Apk apk) { File obbdir = getObbDir(apk.packageName); FileFilter filter = new RegexFileFilter("(main|patch)\\.[0-9-][0-9]*\\." + apk.packageName + "\\.obb"); @@ -743,6 +745,7 @@ public class App extends ValueObject implements Comparable, Parcelable { } } + @SuppressWarnings("EmptyForIteratorPad") private void initInstalledApk(Context context, Apk apk, PackageInfo packageInfo, SanitizedFile apkFile) throws IOException, CertificateEncodingException { apk.compatible = true; @@ -775,7 +778,7 @@ public class App extends ValueObject implements Comparable, Parcelable { JarFile apkJar = new JarFile(apkFile); HashSet abis = new HashSet<>(3); Pattern pattern = Pattern.compile("^lib/([a-z0-9-]+)/.*"); - for (Enumeration jarEntries = apkJar.entries(); jarEntries.hasMoreElements();) { + for (Enumeration jarEntries = apkJar.entries(); jarEntries.hasMoreElements(); ) { JarEntry jarEntry = jarEntries.nextElement(); Matcher matcher = pattern.matcher(jarEntry.getName()); if (matcher.matches()) { diff --git a/app/src/main/java/org/fdroid/fdroid/localrepo/CacheSwapAppsService.java b/app/src/main/java/org/fdroid/fdroid/localrepo/CacheSwapAppsService.java index 885c8bee2..f6452c1eb 100644 --- a/app/src/main/java/org/fdroid/fdroid/localrepo/CacheSwapAppsService.java +++ b/app/src/main/java/org/fdroid/fdroid/localrepo/CacheSwapAppsService.java @@ -74,8 +74,10 @@ public class CacheSwapAppsService extends IntentService { try { PackageManager pm = getPackageManager(); String packageName = intent.getData().getSchemeSpecificPart(); - App app = new App(this, pm, packageName); - SwapService.putAppInCache(packageName, app); + App app = App.getInstance(this, pm, packageName); + if (app != null) { + SwapService.putAppInCache(packageName, app); + } } catch (CertificateEncodingException | IOException | PackageManager.NameNotFoundException e) { e.printStackTrace(); } diff --git a/app/src/main/java/org/fdroid/fdroid/localrepo/LocalRepoManager.java b/app/src/main/java/org/fdroid/fdroid/localrepo/LocalRepoManager.java index 6c0071054..7ea5905cc 100644 --- a/app/src/main/java/org/fdroid/fdroid/localrepo/LocalRepoManager.java +++ b/app/src/main/java/org/fdroid/fdroid/localrepo/LocalRepoManager.java @@ -259,9 +259,9 @@ public final class LocalRepoManager { try { app = SwapService.getAppFromCache(packageName); if (app == null) { - app = new App(context.getApplicationContext(), pm, packageName); + app = App.getInstance(context.getApplicationContext(), pm, packageName); } - if (!app.isValid()) { + if (app == null || !app.isValid()) { return; } } catch (PackageManager.NameNotFoundException | CertificateEncodingException | IOException e) { diff --git a/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java b/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java index 3a16ea723..826de3736 100644 --- a/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java +++ b/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java @@ -94,7 +94,7 @@ public class SwapService extends Service { return INSTALLED_APPS.get(packageName); } - static void putAppInCache(String packageName, App app) { + static void putAppInCache(String packageName, @NonNull App app) { INSTALLED_APPS.put(packageName, app); }