From 5e80e699c4369d3f4604728952ac3d4bc0bae23a Mon Sep 17 00:00:00 2001 From: Jean Chalard Date: Wed, 4 Dec 2013 12:47:05 +0900 Subject: [PATCH] [RF1] Remove files that don't match the expected format, step 1 This implements the skeleton implementation, and enables a fallback implementation in the case the file is coming from the dictionary provider. - A better scheme should be used for provider-supplied dicts. - This does not implement the solution for device-generated dicts yet. This will come in a future change. - This does not implement the checking process on the native side yet. This will come in a future change. Bug: 11281748 Change-Id: Ifa6e237d19dfbffe503e3674915480ea867b0ddf --- .../inputmethod/latin/AssetFileAddress.java | 8 +++ .../inputmethod/latin/BinaryDictionary.java | 7 ++- .../latin/BinaryDictionaryFileDumper.java | 20 +++++-- .../inputmethod/latin/DictionaryFactory.java | 52 ++++++++++++++++++- .../latin/ReadOnlyBinaryDictionary.java | 9 ++++ ...oid_inputmethod_latin_BinaryDictionary.cpp | 13 +++++ 6 files changed, 102 insertions(+), 7 deletions(-) diff --git a/java/src/com/android/inputmethod/latin/AssetFileAddress.java b/java/src/com/android/inputmethod/latin/AssetFileAddress.java index 875192554..855a8d8c7 100644 --- a/java/src/com/android/inputmethod/latin/AssetFileAddress.java +++ b/java/src/com/android/inputmethod/latin/AssetFileAddress.java @@ -52,4 +52,12 @@ public final class AssetFileAddress { if (!f.isFile()) return null; return new AssetFileAddress(filename, offset, length); } + + public boolean pointsToPhysicalFile() { + return 0 == mOffset; + } + + public void deleteUnderlyingFile() { + new File(mFilename).delete(); + } } diff --git a/java/src/com/android/inputmethod/latin/BinaryDictionary.java b/java/src/com/android/inputmethod/latin/BinaryDictionary.java index 174bbfb8e..438f67e35 100644 --- a/java/src/com/android/inputmethod/latin/BinaryDictionary.java +++ b/java/src/com/android/inputmethod/latin/BinaryDictionary.java @@ -121,6 +121,7 @@ public final class BinaryDictionary extends Dictionary { String[] attributeKeyStringArray, String[] attributeValueStringArray); private static native long openNative(String sourceDir, long dictOffset, long dictSize, boolean isUpdatable); + private static native boolean hasValidContentsNative(long dict); private static native void flushNative(long dict, String filePath); private static native boolean needsToRunGCNative(long dict, boolean mindsBlockByGC); private static native void flushWithGCNative(long dict, String filePath); @@ -242,6 +243,10 @@ public final class BinaryDictionary extends Dictionary { return mNativeDict != 0; } + public boolean hasValidContents() { + return hasValidContentsNative(mNativeDict); + } + public int getFormatVersion() { return getFormatVersionNative(mNativeDict); } @@ -376,7 +381,7 @@ public final class BinaryDictionary extends Dictionary { private void reopen() { close(); final File dictFile = new File(mDictFilePath); - // WARNING: Because we pass 0 as the offstet and file.length() as the length, this can + // WARNING: Because we pass 0 as the offset and file.length() as the length, this can // only be called for actual files. Right now it's only called by the flush() family of // functions, which require an updatable dictionary, so it's okay. But beware. loadDictionary(dictFile.getAbsolutePath(), 0 /* startOffset */, diff --git a/java/src/com/android/inputmethod/latin/BinaryDictionaryFileDumper.java b/java/src/com/android/inputmethod/latin/BinaryDictionaryFileDumper.java index ad94a0493..b4382bc2c 100644 --- a/java/src/com/android/inputmethod/latin/BinaryDictionaryFileDumper.java +++ b/java/src/com/android/inputmethod/latin/BinaryDictionaryFileDumper.java @@ -98,7 +98,7 @@ public final class BinaryDictionaryFileDumper { * This creates a URI builder able to build a URI pointing to the dictionary * pack content provider for a specific dictionary id. */ - private static Uri.Builder getProviderUriBuilder(final String path) { + public static Uri.Builder getProviderUriBuilder(final String path) { return new Uri.Builder().scheme(ContentResolver.SCHEME_CONTENT) .authority(DictionaryPackConstants.AUTHORITY).appendPath(path); } @@ -339,15 +339,25 @@ public final class BinaryDictionaryFileDumper { Log.e(TAG, "Could not copy a word list. Will not be able to use it."); // If we can't copy it we should warn the dictionary provider so that it can mark it // as invalid. - wordListUriBuilder.appendQueryParameter(QUERY_PARAMETER_DELETE_RESULT, - QUERY_PARAMETER_FAILURE); + reportBrokenFileToDictionaryProvider(providerClient, clientId, wordlistId); + } + + public static boolean reportBrokenFileToDictionaryProvider( + final ContentProviderClient providerClient, final String clientId, + final String wordlistId) { try { + final Uri.Builder wordListUriBuilder = getContentUriBuilderForType(clientId, + providerClient, QUERY_PATH_DATAFILE, wordlistId /* extraPath */); + wordListUriBuilder.appendQueryParameter(QUERY_PARAMETER_DELETE_RESULT, + QUERY_PARAMETER_FAILURE); if (0 >= providerClient.delete(wordListUriBuilder.build(), null, null)) { - Log.e(TAG, "In addition, we were unable to delete it."); + Log.e(TAG, "Unable to delete a word list."); } } catch (RemoteException e) { - Log.e(TAG, "In addition, communication with the dictionary provider was cut", e); + Log.e(TAG, "Communication with the dictionary provider was cut", e); + return false; } + return true; } // Ideally the two following methods should be merged, but AssetFileDescriptor does not diff --git a/java/src/com/android/inputmethod/latin/DictionaryFactory.java b/java/src/com/android/inputmethod/latin/DictionaryFactory.java index 828e54f14..bcb38da38 100644 --- a/java/src/com/android/inputmethod/latin/DictionaryFactory.java +++ b/java/src/com/android/inputmethod/latin/DictionaryFactory.java @@ -16,6 +16,7 @@ package com.android.inputmethod.latin; +import android.content.ContentProviderClient; import android.content.Context; import android.content.res.AssetFileDescriptor; import android.content.res.Resources; @@ -62,8 +63,12 @@ public final class DictionaryFactory { final ReadOnlyBinaryDictionary readOnlyBinaryDictionary = new ReadOnlyBinaryDictionary(f.mFilename, f.mOffset, f.mLength, useFullEditDistance, locale, Dictionary.TYPE_MAIN); - if (readOnlyBinaryDictionary.isValidDictionary()) { + if (readOnlyBinaryDictionary.hasValidContents()) { dictList.add(readOnlyBinaryDictionary); + } else { + readOnlyBinaryDictionary.close(); + // Prevent this dictionary to do any further harm. + killDictionary(context, f); } } } @@ -74,6 +79,51 @@ public final class DictionaryFactory { return new DictionaryCollection(Dictionary.TYPE_MAIN, dictList); } + /** + * Kills a dictionary so that it is never used again, if possible. + * @param context The context to contact the dictionary provider, if possible. + * @param f A file address to the dictionary to kill. + */ + private static void killDictionary(final Context context, final AssetFileAddress f) { + if (f.pointsToPhysicalFile()) { + f.deleteUnderlyingFile(); + // Warn the dictionary provider if the dictionary came from there. + final ContentProviderClient providerClient; + try { + providerClient = context.getContentResolver().acquireContentProviderClient( + BinaryDictionaryFileDumper.getProviderUriBuilder("").build()); + } catch (final SecurityException e) { + Log.e(TAG, "No permission to communicate with the dictionary provider", e); + return; + } + if (null == providerClient) { + Log.e(TAG, "Can't establish communication with the dictionary provider"); + return; + } + final String wordlistId = + DictionaryInfoUtils.getWordListIdFromFileName(new File(f.mFilename).getName()); + if (null != wordlistId) { + // TODO: this is a reasonable last resort, but it is suboptimal. + // The following will remove the entry for this dictionary with the dictionary + // provider. When the metadata is downloaded again, we will try downloading it + // again. + // However, in the practice that will mean the user will find themselves without + // the new dictionary. That's fine for languages where it's included in the APK, + // but for other languages it will leave the user without a dictionary at all until + // the next update, which may be a few days away. + // Ideally, we would trigger a new download right away, and use increasing retry + // delays for this particular id/version combination. + // Then again, this is expected to only ever happen in case of human mistake. If + // the wrong file is on the server, the following is still doing the right thing. + // If it's a file left over from the last version however, it's not great. + BinaryDictionaryFileDumper.reportBrokenFileToDictionaryProvider( + providerClient, + context.getString(R.string.dictionary_pack_client_id), + wordlistId); + } + } + } + /** * Initializes a main dictionary collection from a dictionary pack, with default flags. * diff --git a/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java index 68505ce38..c8e4014bb 100644 --- a/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java +++ b/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java @@ -44,6 +44,15 @@ public final class ReadOnlyBinaryDictionary extends Dictionary { locale, dictType, false /* isUpdatable */); } + public boolean hasValidContents() { + mLock.readLock().lock(); + try { + return mBinaryDictionary.hasValidContents(); + } finally { + mLock.readLock().unlock(); + } + } + public boolean isValidDictionary() { return mBinaryDictionary.isValidDictionary(); } diff --git a/native/jni/com_android_inputmethod_latin_BinaryDictionary.cpp b/native/jni/com_android_inputmethod_latin_BinaryDictionary.cpp index c6a5900e7..87d482be1 100644 --- a/native/jni/com_android_inputmethod_latin_BinaryDictionary.cpp +++ b/native/jni/com_android_inputmethod_latin_BinaryDictionary.cpp @@ -135,6 +135,14 @@ static void latinime_BinaryDictionary_close(JNIEnv *env, jclass clazz, jlong dic delete dictionary; } +static bool latinime_BinaryDictionary_hasValidContents(JNIEnv *env, jclass clazz, + jlong dict) { + Dictionary *dictionary = reinterpret_cast(dict); + if (!dictionary) return false; + // TODO: check format version + return true; +} + static int latinime_BinaryDictionary_getFormatVersion(JNIEnv *env, jclass clazz, jlong dict) { Dictionary *dictionary = reinterpret_cast(dict); if (!dictionary) return 0; @@ -437,6 +445,11 @@ static const JNINativeMethod sMethods[] = { const_cast("(J)V"), reinterpret_cast(latinime_BinaryDictionary_close) }, + { + const_cast("hasValidContentsNative"), + const_cast("(J)Z"), + reinterpret_cast(latinime_BinaryDictionary_hasValidContents) + }, { const_cast("getFormatVersionNative"), const_cast("(J)I"),