Report invalid SQL with non-crashing 'wtf' rather than crash

- Also allow the use of "default_directory".

Test: unit tests
Bug 31801512

Change-Id: I9261c6e71fdd96449c98ef62084cfe0b21419f9a

4 files changed, 58 insertions, 41 deletions

diff --git a/src/com/android/providers/contacts/ContactsDatabaseHelper.java b/src/com/android/providers/contacts/ContactsDatabaseHelper.java
index e827df33..7d54c26f 100644
--- a/src/com/android/providers/contacts/ContactsDatabaseHelper.java
+++ b/src/com/android/providers/contacts/ContactsDatabaseHelper.java
@@ -18,6 +18,7 @@ package com.android.providers.contacts;
 
 import com.android.providers.contacts.sqlite.DatabaseAnalyzer;
 import com.android.providers.contacts.sqlite.SqlChecker;
+import com.android.providers.contacts.sqlite.SqlChecker.InvalidSqlException;
 import com.android.providers.contacts.util.PropertyUtils;
 import com.google.android.collect.Sets;
 import com.google.common.annotations.VisibleForTesting;
@@ -85,6 +86,7 @@ import android.text.util.Rfc822Token;
 import android.text.util.Rfc822Tokenizer;
 import android.util.Base64;
 import android.util.Log;
+import android.util.Slog;
 
 import com.android.common.content.SyncStateContentProviderHelper;
 import com.android.providers.contacts.aggregation.util.CommonNicknameCache;
@@ -977,6 +979,7 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
 
     private final Context mContext;
     private final boolean mDatabaseOptimizationEnabled;
+    private final boolean mIsTestInstance;
     private final SyncStateContentProviderHelper mSyncState;
     private final CountryMonitor mCountryMonitor;
 
@@ -1005,11 +1008,10 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
     private CharArrayBuffer mCharArrayBuffer = new CharArrayBuffer(128);
     private NameSplitter mNameSplitter;
 
-    private volatile boolean mEnableSqlCheck = false;
-
     public static synchronized ContactsDatabaseHelper getInstance(Context context) {
         if (sSingleton == null) {
-            sSingleton = new ContactsDatabaseHelper(context, DATABASE_NAME, true);
+            sSingleton = new ContactsDatabaseHelper(context, DATABASE_NAME, true,
+                    /* isTestInstance=*/ false);
         }
         return sSingleton;
     }
@@ -1019,13 +1021,15 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
      */
     @NeededForTesting
     public static ContactsDatabaseHelper getNewInstanceForTest(Context context) {
-        return new ContactsDatabaseHelper(context, null, false);
+        return new ContactsDatabaseHelper(context, null, false, /* isTestInstance=*/ true);
     }
 
     protected ContactsDatabaseHelper(
-            Context context, String databaseName, boolean optimizationEnabled) {
+            Context context, String databaseName, boolean optimizationEnabled,
+            boolean isTestInstance) {
         super(context, databaseName, null, DATABASE_VERSION);
         mDatabaseOptimizationEnabled = optimizationEnabled;
+        mIsTestInstance = isTestInstance;
         Resources resources = context.getResources();
 
         mContext = context;
@@ -6184,10 +6188,6 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
         return metadataSyncInsert.executeInsert();
     }
 
-    public void setSqlCheckEnabled(boolean enabled) {
-        mEnableSqlCheck = enabled;
-    }
-
     private SqlChecker mCachedSqlChecker;
 
     private SqlChecker getSqlChecker() {
@@ -6205,7 +6205,8 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
         // Disallow token "select" to disallow subqueries.
         invalidTokens.add("select");
 
-        // TODO Add hidden columns if needed.
+        // Allow the use of "default_directory" for now, as it used to be sort of commonly used...
+        invalidTokens.remove(Tables.DEFAULT_DIRECTORY.toLowerCase());
 
         mCachedSqlChecker = new SqlChecker(invalidTokens);
 
@@ -6215,31 +6216,50 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper {
     /**
      * Ensure (a piece of) SQL is valid and doesn't contain disallowed tokens.
      */
-    public void validateSql(String sqlPiece) {
-        if (mEnableSqlCheck) {
-            getSqlChecker().ensureNoInvalidTokens(sqlPiece);
-        }
+    public void validateSql(String callerPackage, String sqlPiece) {
+        runSqlValidation(callerPackage, () -> getSqlChecker().ensureNoInvalidTokens(sqlPiece));
     }
 
     /**
      * Ensure all keys in {@code values} are valid. (i.e. they're all single token.)
      */
-    public void validateContentValues(ContentValues values) {
-        if (mEnableSqlCheck) {
+    public void validateContentValues(String callerPackage, ContentValues values) {
+        runSqlValidation(callerPackage, () -> {
             for (String key : values.keySet()) {
                 getSqlChecker().ensureSingleTokenOnly(key);
             }
-        }
-    }
+        });
+   }
 
     /**
      * Ensure all column names in {@code projection} are valid. (i.e. they're all single token.)
      */
-    public void validateProjection(String[] projection) {
-        if (mEnableSqlCheck && projection != null) {
-            for (String column : projection) {
-                getSqlChecker().ensureSingleTokenOnly(column);
-            }
+    public void validateProjection(String callerPackage, String[] projection) {
+        if (projection != null) {
+            runSqlValidation(callerPackage, () -> {
+                for (String column : projection) {
+                    getSqlChecker().ensureSingleTokenOnly(column);
+                }
+            });
+        }
+    }
+
+    private void runSqlValidation(String callerPackage, Runnable r) {
+        // STOPSHIP Allow certain system apps to access it
+        try {
+            r.run();
+        } catch (InvalidSqlException e) {
+            reportInvalidSql(callerPackage, e);
+        }
+    }
+
+    private void reportInvalidSql(String callerPackage, InvalidSqlException e) {
+        final String message = String.format("%s caller=%s", e.getMessage(), callerPackage);
+        if (mIsTestInstance) {
+            Slog.w(TAG, "[Test mode, warning only] " + message);
+        } else {
+            Slog.wtfStack(TAG, message);
         }
+        throw e; // STOPSHIP Don't throw for pre-O apps.
     }
 }
diff --git a/src/com/android/providers/contacts/ContactsProvider2.java b/src/com/android/providers/contacts/ContactsProvider2.java
index c52cd584..a2b1e7b4 100644
--- a/src/com/android/providers/contacts/ContactsProvider2.java
+++ b/src/com/android/providers/contacts/ContactsProvider2.java
@@ -1582,12 +1582,7 @@ public class ContactsProvider2 extends AbstractContactsProvider
         mMetadataSyncEnabled = android.provider.Settings.Global.getInt(
                 getContext().getContentResolver(), Global.CONTACT_METADATA_SYNC_ENABLED, 0) == 1;
 
-        // STOPSHIP Move the constant to Settings.
-        final boolean enableSqlCheck = android.provider.Settings.Global.getInt(
-                getContext().getContentResolver(), "contact_sql_check_enabled", 1) == 1;
-
         mContactsHelper = getDatabaseHelper(getContext());
-        mContactsHelper.setSqlCheckEnabled(enableSqlCheck);
         mDbHelper.set(mContactsHelper);
 
         // Set up the DB helper for keeping transactions serialized.
@@ -2245,7 +2240,7 @@ public class ContactsProvider2 extends AbstractContactsProvider
     public Uri insert(Uri uri, ContentValues values) {
         waitForAccess(mWriteAccessLatch);
 
-        mContactsHelper.validateContentValues(values);
+        mContactsHelper.validateContentValues(getCallingPackage(), values);
 
         if (mapsToProfileDbWithInsertedValues(uri, values)) {
             switchToProfileMode();
@@ -2259,8 +2254,8 @@ public class ContactsProvider2 extends AbstractContactsProvider
     public int update(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
         waitForAccess(mWriteAccessLatch);
 
-        mContactsHelper.validateContentValues(values);
-        mContactsHelper.validateSql(selection);
+        mContactsHelper.validateContentValues(getCallingPackage(), values);
+        mContactsHelper.validateSql(getCallingPackage(), selection);
 
         if (mapsToProfileDb(uri)) {
             switchToProfileMode();
@@ -2274,7 +2269,7 @@ public class ContactsProvider2 extends AbstractContactsProvider
     public int delete(Uri uri, String selection, String[] selectionArgs) {
         waitForAccess(mWriteAccessLatch);
 
-        mContactsHelper.validateSql(selection);
+        mContactsHelper.validateSql(getCallingPackage(), selection);
 
         if (mapsToProfileDb(uri)) {
             switchToProfileMode();
@@ -5508,9 +5503,9 @@ public class ContactsProvider2 extends AbstractContactsProvider
                     " User=" + UserUtils.getCurrentUserHandle(getContext()));
         }
 
-        mContactsHelper.validateProjection(projection);
-        mContactsHelper.validateSql(selection);
-        mContactsHelper.validateSql(sortOrder);
+        mContactsHelper.validateProjection(getCallingPackage(), projection);
+        mContactsHelper.validateSql(getCallingPackage(), selection);
+        mContactsHelper.validateSql(getCallingPackage(), sortOrder);
 
         waitForAccess(mReadAccessLatch);
 
diff --git a/src/com/android/providers/contacts/ProfileDatabaseHelper.java b/src/com/android/providers/contacts/ProfileDatabaseHelper.java
index a23e5217..56280916 100644
--- a/src/com/android/providers/contacts/ProfileDatabaseHelper.java
+++ b/src/com/android/providers/contacts/ProfileDatabaseHelper.java
@@ -43,17 +43,19 @@ public class ProfileDatabaseHelper extends ContactsDatabaseHelper {
      */
     @NeededForTesting
     public static ProfileDatabaseHelper getNewInstanceForTest(Context context) {
-        return new ProfileDatabaseHelper(context, null, false);
+        return new ProfileDatabaseHelper(context, null, false, /* isTestInstance=*/ true);
     }
 
     private ProfileDatabaseHelper(
-            Context context, String databaseName, boolean optimizationEnabled) {
-        super(context, databaseName, optimizationEnabled);
+            Context context, String databaseName, boolean optimizationEnabled,
+            boolean isTestInstance) {
+        super(context, databaseName, optimizationEnabled, isTestInstance);
     }
 
     public static synchronized ProfileDatabaseHelper getInstance(Context context) {
         if (sSingleton == null) {
-            sSingleton = new ProfileDatabaseHelper(context, DATABASE_NAME, true);
+            sSingleton = new ProfileDatabaseHelper(context, DATABASE_NAME, true,
+                    /* isTestInstance=*/ false);
         }
         return sSingleton;
     }
diff --git a/src/com/android/providers/contacts/sqlite/DatabaseAnalyzer.java b/src/com/android/providers/contacts/sqlite/DatabaseAnalyzer.java
index 63502214..9a03aaf9 100644
--- a/src/com/android/providers/contacts/sqlite/DatabaseAnalyzer.java
+++ b/src/com/android/providers/contacts/sqlite/DatabaseAnalyzer.java
@@ -44,7 +44,7 @@ public class DatabaseAnalyzer {
         try (final Cursor c = db.rawQuery(
                 "SELECT name FROM sqlite_master WHERE type in (\"table\", \"view\")", null)) {
             while (c.moveToNext()) {
-                ret.add(c.getString(0));
+                ret.add(c.getString(0).toLowerCase());
             }
         }
         return ret;
@@ -61,7 +61,7 @@ public class DatabaseAnalyzer {
         try {
             // Collect the column names.
             for (int i = 0; i < c.getColumnCount(); i++) {
-                ret.add(c.getColumnName(i));
+                ret.add(c.getColumnName(i).toLowerCase());
             }
         } finally {
             c.close();