sqlcipher / android-database-sqlcipher

Android SQLite API based on SQLCipher
https://www.zetetic.net/sqlcipher/sqlcipher-for-android/
Other
2.73k stars 564 forks source link

net.sqlcipher.database.SQLiteDatabase.native_execSQL #555

Closed allenhsu-17live closed 2 years ago

allenhsu-17live commented 2 years ago

Expected Behavior

No crash

Actual Behavior

crash happen from firebase

net.sqlcipher.database.SQLiteDatabase.native_execSQL (SQLiteDatabase.java)
net.sqlcipher.database.SQLiteDatabase.execSQL (SQLiteDatabase.java:2439)
net.sqlcipher.database.SQLiteDatabase.beginTransactionWithListenerInternal (SQLiteDatabase.java:3052)
net.sqlcipher.database.SQLiteDatabase.beginTransactionWithListener (SQLiteDatabase.java:777)
net.sqlcipher.database.SQLiteDatabase.beginTransaction (SQLiteDatabase.java:748)
androidx.room.InvalidationTracker.syncTriggers (InvalidationTracker.java:498)
androidx.room.InvalidationTracker.syncTriggers (InvalidationTracker.java:538)
androidx.room.InvalidationTracker.addObserver (InvalidationTracker.java:275)
androidx.room.MultiInstanceInvalidationClient$3.run (MultiInstanceInvalidationClient.java:124)
java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1167)
java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:641)
java.lang.Thread.run (Thread.java:764)

I guess the rootcase on here

void syncTriggers(SupportSQLiteDatabase database) {
        if (database.inTransaction()) {
            // we won't run this inside another transaction.
            return;
        }
        try {
            // This method runs in a while loop because while changes are synced to db, another
            // runnable may be skipped. If we cause it to skip, we need to do its work.
            while (true) {
                Lock closeLock = mDatabase.getCloseLock();
                closeLock.lock();
                try {
                    // there is a potential race condition where another mSyncTriggers runnable
                    // can start running right after we get the tables list to sync.
                    final int[] tablesToSync = mObservedTableTracker.getTablesToSync();
                    if (tablesToSync == null) {
                        return;
                    }
                    final int limit = tablesToSync.length;
                    database.beginTransaction();
                    try {
                        for (int tableId = 0; tableId < limit; tableId++) {
                            switch (tablesToSync[tableId]) {
                                case ObservedTableTracker.ADD:
                                    startTrackingTable(database, tableId);
                                    break;
                                case ObservedTableTracker.REMOVE:
                                    stopTrackingTable(database, tableId);
                                    break;
                            }
                        }
                        database.setTransactionSuccessful();
                    } finally {
                        database.endTransaction();
                    }
                    mObservedTableTracker.onSyncCompleted();
                } finally {
                    closeLock.unlock();
                }
            }
        } catch (IllegalStateException | SQLiteException exception) {
            // may happen if db is closed. just log.
            Log.e(Room.LOG_TAG, "Cannot run invalidation tracker. Is the db closed?",
                    exception);
        }
    }

the roomdata base catch SQLiteException exception, but the full class is android.database.sqlite.SQLiteException But when I use net.sqlcipher.database, will throw this exception net.sqlcipher.database.SQLiteException It's different class. so Room can't try catch that. could you try to fix it?

Steps to Reproduce

  1. Use the RoomDataBase
  2. enableMultiInstanceInvalidation (Because we need multi process)

SQLCipher version (can be identified by executing PRAGMA cipher_version;):

SQLCipher for Android version: 4.4.3 Are you able to reproduce this issue within the SQLCipher for Android test suite? I can't, just only firebase report

Note: If you are not posting a specific issue for the SQLCipher library, please post your question to the SQLCipher discuss site. Thanks!

allenhsu-17live commented 2 years ago

like this pr

https://github.com/sqlcipher/android-database-sqlcipher/pull/556

developernotes commented 2 years ago

Hi @allenhsu-17live

From your stack trace it appears the exception is thrown from the call to database.beginTransaction();, specifically here. Have you tried to create a new test case within the SQLCipher for Android test suite that showcases the behavior? Renaming the exception that is thrown doesn't resolve the root issue you're experiencing from what I can see.

allenhsu-17live commented 2 years ago

Him @developernotes I can't , because the report from online. But I think we should consider use android.database.sqlite exception to replace all. right?

and even cause in this line database.beginTransaction();

I wrote the mock to test

    try {
        try {
            beginTransactionMock()
        } finally {
            closeLockMoc()
        }
    } catch (e: SQLException) {
        print("dosomething")
        //dosomething
    }

    fun beginTransactionMock() {
    throw SQLException("beginTransactionMock")
}

    fun closeLockMoc() {
    //    throw SQLException("closeLockMoc")
    }

The outermost try catch will still catch the error. will print dosomething but if you use different packageNam

    try {
        try {
            beginTransactionMock()
        } finally {
            closeLockMoc()
        }
    } catch (e: SQLException) {
        print("dosomething")
        //dosomething
    }

    fun beginTransactionMock() {
    throw mock.SQLException("beginTransactionMock")
}

    fun closeLockMoc() {
    //    throw mock.SQLException("closeLockMoc")
    }

will be get crash

developernotes commented 2 years ago

Hi @allenhsu-17live

I understand the catch logic you are suggesting, however, it doesn't address why the exception is being thrown in the first place. As to the rename, it may be possible to make an adjustment to the exception hierarchy without renaming. This is something we will consider.

allenhsu-17live commented 2 years ago

@developernotes Ok, got it, But regardless of the root cause, I still suggest that you should use the official android exception instead of a copy. You can refer to the PR I mentioned

stale[bot] commented 2 years ago

Hello, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "bug", "enhancement", or "security" and I will leave it open. Thank you for your contributions.

stale[bot] commented 2 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to reopen with up-to-date information.