Bundled SQLite crashes due to thread safe mode [340949940]

Fixed

Bug

Status Update

No update yet.

Description

ib...@gmail.com

created issue #1

May 16, 2024 05:02AM

Component used: bundled androidx.sqlite:sqlite-bundled
Version used: 2.5.0-alpha02
Devices/Android versions reproduced on: Pixel 6

I tried to migrate from

https://github.com/requery/sqlite-android to this library and I noticed that my queries started to cause app crashes from time to time. Here is the Logcat output:

2024-05-16 11:41:26.367 30919-30967 libc org.btcmap.debug A Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x8000004300003b in tid 30967 (DefaultDispatch), pid 30919 (rg.btcmap.debug)

2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A Cmdline: org.btcmap.debug
2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A pid: 30919, tid: 30967, name: DefaultDispatch >>> org.btcmap.debug <<<
2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A #00 pc 00000000000cb30c /data/app/~~98VGkyL4jH13WZ5B1K-MmQ==/org.btcmap.debug-MmWOzd6cam3KXcL8NYi-uA==/base.apk!libsqliteJni.so (offset 0x930000)

The app itself is open source and it doesn't need any extra setup, you can easily reproduce it by launching it in the emulator or on a real device. Usually it crashes before moving the map, but it will crash for sure if you start moving the map, since it triggers a few queries.

https://github.com/teambtcmap/btcmap-android/commit/1d60da058a10ed1efaff307af4bcf3e5378fc2a2

It's possible that I messed up a few queries during that transition, but I don't think it should cause crashes, and it can surely be more specific on where it's failing and why

Comments

da...@google.com <da...@google.com> #2May 16, 2024 12:34PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

https://android-review.googlesource.com/1123258

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

ib...@gmail.com <ib...@gmail.com> #3May 16, 2024 01:01PM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

da...@google.com <da...@google.com> #4May 16, 2024 01:52PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

https://android-review.googlesource.com/1288456

ib...@gmail.com <ib...@gmail.com> #5May 17, 2024 12:04PM

Ah, I see, thanks for clarification. I was confused by this line:

The default mode is serialized. (https://www.sqlite.org/threadsafe.html)

So I assumed that it's perfectly safe to share the connection between the IO thread pool, but it seems like it's not the case for androidx.sqlite:sqlite-bundled. Any specific reason for overriding the defaults? Using the safest option by default while also allowing runtime overrides seems like a good idea

da...@google.com <da...@google.com> #6May 30, 2024 03:49PM

We compile with the multi-thread flag for performance reasons as Room has an internal connection pool.

I have a change that adds an overload to open() such that open flags can be passed, including SQLITE_OPEN_FULLMUTEX which will enable serialized mode for the connection, see example here.

ap...@google.com <ap...@google.com> #7May 31, 2024 08:37PM

Project: platform/frameworks/support
Branch: androidx-main

commit 70731ee26381fb7a93cbfd3b822f35f14a75a5b1
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Thu May 30 11:45:50 2024

Add an open() overload that receives open flags to bundled driver.

Bug: 307917398
Bug: 340949940
Test: BaseBundledConformanceTest
Relnote: Add an open() overload API to BundledSQLiteDriver to pass open flags when opening a database connection. Useful for opening a database in read-only mode or using the serialized thread safe mode instead of the multi-thread mode bundled SQLite is compiled with.
Change-Id: I1ae858f70db811c9e08aeb0c171a20a2f9e2a28c

M sqlite/integration-tests/driver-conformance-test/build.gradle
M sqlite/integration-tests/driver-conformance-test/src/androidInstrumentedTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/integration-tests/driver-conformance-test/src/commonTest/kotlin/androidx/sqlite/driver/test/BaseBundledConformanceTest.kt
M sqlite/integration-tests/driver-conformance-test/src/jvmTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/integration-tests/driver-conformance-test/src/nativeTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/sqlite-bundled/api/api_lint.ignore
M sqlite/sqlite-bundled/api/current.txt
M sqlite/sqlite-bundled/api/restricted_current.txt
M sqlite/sqlite-bundled/src/androidJvmCommonMain/jni/sqlite_bindings.cpp
M sqlite/sqlite-bundled/src/androidJvmCommonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.androidJvmCommon.kt
M sqlite/sqlite-bundled/src/androidJvmCommonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLiteDriver.androidJvmCommon.kt
A sqlite/sqlite-bundled/src/commonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.kt
M sqlite/sqlite-bundled/src/commonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLiteDriver.kt
A sqlite/sqlite-bundled/src/nativeMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.nativeCommon.kt
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLite.kt
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLiteDriver.kt

https://android-review.googlesource.com/3106942

da...@google.com <da...@google.com> May 31, 2024 08:38PM

Marked as fixed.

ap...@google.com <ap...@google.com> #8Jun 3, 2024 05:52PM

Project: platform/frameworks/support
Branch: androidx-main

commit 379e8f8049ea93808f284c4b46b83e9692f3d2d7
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Thu May 30 11:45:50 2024

Add an open() overload that receives open flags to bundled driver.

Bug: 307917398
Bug: 340949940
Test: BaseBundledConformanceTest
Relnote: Add an open() overload API to BundledSQLiteDriver to pass open flags when opening a database connection. Useful for opening a database in read-only mode or using the serialized thread safe mode instead of the multi-thread mode bundled SQLite is compiled with.
Change-Id: I1fe0e1a85ff555a7950165eefe0aa0301ad05a94

M sqlite/integration-tests/driver-conformance-test/build.gradle
M sqlite/integration-tests/driver-conformance-test/src/androidInstrumentedTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/integration-tests/driver-conformance-test/src/commonTest/kotlin/androidx/sqlite/driver/test/BaseBundledConformanceTest.kt
M sqlite/integration-tests/driver-conformance-test/src/jvmTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/integration-tests/driver-conformance-test/src/nativeTest/kotlin/androidx/sqlite/driver/test/BundledSQLiteDriverTest.kt
M sqlite/sqlite-bundled/api/api_lint.ignore
M sqlite/sqlite-bundled/api/current.txt
M sqlite/sqlite-bundled/api/restricted_current.txt
M sqlite/sqlite-bundled/src/androidJvmCommonMain/jni/sqlite_bindings.cpp
M sqlite/sqlite-bundled/src/androidJvmCommonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.androidJvmCommon.kt
M sqlite/sqlite-bundled/src/androidJvmCommonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLiteDriver.androidJvmCommon.kt
A sqlite/sqlite-bundled/src/commonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.kt
M sqlite/sqlite-bundled/src/commonMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLiteDriver.kt
A sqlite/sqlite-bundled/src/nativeMain/kotlin/androidx/sqlite/driver/bundled/BundledSQLite.nativeCommon.kt
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLite.kt
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLiteDriver.kt

https://android-review.googlesource.com/3110759

pr...@google.com <pr...@google.com> #9Jun 12, 2024 10:45PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.sqlite:sqlite-framework-iosarm64:2.5.0-alpha04
androidx.sqlite:sqlite-framework-iossimulatorarm64:2.5.0-alpha04
androidx.sqlite:sqlite-framework-iosx64:2.5.0-alpha04
androidx.sqlite:sqlite-framework-linuxx64:2.5.0-alpha04
androidx.sqlite:sqlite-framework-macosarm64:2.5.0-alpha04
androidx.sqlite:sqlite-framework-macosx64:2.5.0-alpha04

ib...@gmail.com <ib...@gmail.com> #10Sep 23, 2024 01:52PM

I just tested this new flag and it looks like the issue still persists. It's easily reproducible since the repo is open source:

https://github.com/teambtcmap/btcmap-android/commit/9fe6be3604067ae1a1bd8b9c576b014023bff6ec

So default open() passes SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE flags, and I changed it to SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE or SQLITE_OPEN_FULLMUTEX

The database works fine after that change, but when I tried to disable the global connection lock (connLock variable) in the same file, the app started to crash with "android.database.SQLException: Error code: 21, message: no row"

android.database.SQLException: Error code: 21, message: no row
    at androidx.sqlite.driver.bundled.BundledSQLiteStatementKt.nativeGetLong(Native Method)
    at androidx.sqlite.driver.bundled.BundledSQLiteStatementKt.access$nativeGetLong(BundledSQLiteStatement.jvmAndroid.kt:1)
    at androidx.sqlite.driver.bundled.BundledSQLiteStatement.getLong(BundledSQLiteStatement.jvmAndroid.kt:70)
    at element.ElementQueries$selectClusters$1.invoke(ElementQueries.kt:271)

da...@google.com <da...@google.com> #11Sep 25, 2024 03:31PM

Status: Assigned (reopened)

The flags are working as expected, but looks like the issue is in our JNI implementation, specifically we use sqlite3_errcode to get the 'last result code' and that is dangerous since it is a connection level API and if the connection is being used concurrently the 'last result code' of a statement might be from a different statement being executed, in other words we need to localize the 'last result code' per statement. I'll work on a fix. Again, I appreciate the feedback and you trying out the driver.

Message last modified on Sep 25, 2024 03:31PM

ap...@google.com <ap...@google.com> #12Sep 26, 2024 07:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Daniel Santiago Rivera <danysantiago@google.com>
Link: https://android-review.googlesource.com/3282515

Use sqlite3_stmt_busy to check if there are rows in a statement.

Expand for full commit details

Use sqlite3_stmt_busy to check if there are rows in a statement.

Instead of using 'sqlite3_errcode(db) == SQLITE_ROW' to check if a statement is in a row and ready to read columns, use sqlite3_stmt_busy as that API is local to the statement and avoids the risk of sqlite3_errcode that works on a database connection returning a result code that is irrelevant to the statement when the connection is being used concurrently.

Bug: 340949940
Test: BaseBundledConformanceTest.openWithFullMutexFlag
Change-Id: If02039b90c6415a7532ae94f1c99e1182b546db8

Files:

M sqlite/integration-tests/driver-conformance-test/src/commonTest/kotlin/androidx/sqlite/driver/test/BaseBundledConformanceTest.kt
M sqlite/sqlite-bundled/src/jvmAndroidMain/jni/sqlite_bindings.cpp
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLiteStatement.kt

Hash: 5885a89d74a47b73d2e05abefc979279b85c14fe
Date: Wed Sep 25 12:13:07 2024

da...@google.com <da...@google.com> Sep 26, 2024 07:08PM