Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sqlite] Fix iOS crash on int overflow #25322

Merged

Conversation

peterferguson
Copy link
Contributor

@peterferguson peterferguson commented Nov 10, 2023

Fixes #25321 by adding a guard in bindStatementParam

@peterferguson peterferguson requested a review from Kudo as a code owner November 10, 2023 11:32
@Kudo
Copy link
Contributor

Kudo commented Nov 10, 2023

thanks @peterferguson for the fix. we actually have a backlog to support BigInt and i think that's a better solution. let me clarify whether i can add BigInt support soon. otherwise i will merge the fix from you. does that sound good to you?

@peterferguson
Copy link
Contributor Author

Perfect that is much better 👍

@Kudo
Copy link
Contributor

Kudo commented Nov 15, 2023

hey @peterferguson! i just remembered that BigInt is still not supported on JSCRuntime. we may need more time to design a proper compatible API for BigInt. so we may better to resolve the crash first. at least to support support integer lower than Number.MAX_SAFE_INTEGER. considering iOS devices are nowadays all in 64 bits, i think we can just use int64. we also use Long to convert sqlite int64 on android. would you like to update the pr for this patch?

diff --git a/apps/test-suite/tests/SQLiteNext.ts b/apps/test-suite/tests/SQLiteNext.ts
index cfe0d3ae8c..18fc06337b 100644
--- a/apps/test-suite/tests/SQLiteNext.ts
+++ b/apps/test-suite/tests/SQLiteNext.ts
@@ -48,6 +48,18 @@ CREATE TABLE IF NOT EXISTS Users (user_id INTEGER PRIMARY KEY NOT NULL, name VAR
       await db.closeAsync();
     });
 
+    it('should support bigger integers than int32_t', async () => {
+      const db = await SQLite.openDatabaseAsync(':memory:');
+      const value = 1700007974511;
+      const row = await db.getAsync<{ value: number }>(`SELECT ${value} as value`);
+      expect(row['value']).toBe(value);
+      const row2 = await db.getAsync<{ value: number }>('SELECT $value as value', {
+        $value: value,
+      });
+      expect(row2['value']).toBe(value);
+      await db.closeAsync();
+    });
+
     it('should support PRAGMA statements', async () => {
       const db = await SQLite.openDatabaseAsync(':memory:');
       await db.execAsync(`
diff --git a/packages/expo-sqlite/ios/SQLiteModuleNext.swift b/packages/expo-sqlite/ios/SQLiteModuleNext.swift
index ea77177b10..ff43231acd 100644
--- a/packages/expo-sqlite/ios/SQLiteModuleNext.swift
+++ b/packages/expo-sqlite/ios/SQLiteModuleNext.swift
@@ -432,7 +432,7 @@ public final class SQLiteModuleNext: Module {
 
     switch type {
     case SQLITE_INTEGER:
-      return sqlite3_column_int(instance, index)
+      return sqlite3_column_int64(instance, index)
     case SQLITE_FLOAT:
       return sqlite3_column_double(instance, index)
     case SQLITE_TEXT:
@@ -460,8 +460,8 @@ public final class SQLiteModuleNext: Module {
       sqlite3_bind_null(instance, index)
     case _ as NSNull:
       sqlite3_bind_null(instance, index)
-    case let param as Int:
-      sqlite3_bind_int(instance, index, Int32(param))
+    case let param as Int64:
+      sqlite3_bind_int64(instance, index, Int64(param))
     case let param as Double:
       sqlite3_bind_double(instance, index, param)
     case let param as String:

@peterferguson
Copy link
Contributor Author

@Kudo Just added the patch code and removed the other test I had written 👍

Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks much @peterferguson for your help 🔥

@Kudo Kudo merged commit 3648eea into expo:main Nov 17, 2023
9 of 10 checks passed
@Kudo Kudo added the published Changes from the PR have been published to npm label Nov 18, 2023
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sqlite] expo-sqlite/next crashes iOS app on int overflow
2 participants