From 6bb135872a80ae4a06ff1a3bc70282c209960e94 Mon Sep 17 00:00:00 2001 From: nigig-ci Date: Fri, 31 Jul 2026 22:21:42 +0000 Subject: [PATCH 1/2] fix(sms): stop the cursor loop aborting the process (A1, A2) Three defects in the content-provider read path, all of which abort rather than return an error, and all of which were duplicated because inbox.rs and thread.rs each carried their own byte-identical copy of the column helpers (~75 lines). A1 -- local reference table overflow. Every get_*_column() called env.new_string(name) to resolve the column by name, and getString returned another local ref: ~10 JNI local references per message row. Local refs are not freed when the Rust value drops; they live until the native method returns to the JVM, which here is the end of the whole cursor loop. ART's default local reference capacity is 512, so the table overflowed at roughly 45-50 messages -- and overflow is a hard abort ("local reference table overflow"), not an Err. Any real inbox has hundreds to thousands of messages. I consider this the most likely single source of field crashes in this crate. Fixed twice over: column indices are resolved ONCE per query into a struct instead of per row per column, which removes the new_string churn entirely; and each row is read inside env.with_local_frame(), so whatever a row does allocate is released when that row finishes. A2 -- pending exceptions were never cleared. My assessment said this crate does no exception checking. That was WRONG, and the correction matters: jni-rs 0.21 expands every checked call through check_exception!, which calls ExceptionCheck and returns Err(Error::JavaException). The actual defect is that nothing ever CLEARS it. ExceptionClear appears in exactly one place in jni-rs -- the public exception_clear() -- and this crate never called it. So the Err propagated while the exception stayed pending on the thread, and the next JNI call made on that thread aborted the process. A SecurityException from a revoked READ_SMS, or an IllegalStateException from a stale cursor, therefore produced a tidy Err and then killed the app somewhere unrelated. Every error path out of these functions now runs clear_pending_exception(), which describes the exception to logcat and leaves the thread clean. Cursor leak -- `?` inside the loop skipped the close() at the end of the function, leaking the Cursor and its CursorWindow (typically a 2 MB ashmem region) on every error. CursorGuard closes it on all paths. Also removes the duplicated helpers: both files now share sys/android/cursor.rs, so a fix here cannot land in one copy and miss the other. Verified: check and clippy -D warnings clean on host and aarch64-linux-android. NOT verified on a device. The abort threshold, the frame capacity and the exception paths need an emulator or handset with a populated inbox to confirm, and there is still no CI runner registered on this repo. --- crates/robius-sms/src/sys/android/cursor.rs | 273 ++++++++++++++++++++ crates/robius-sms/src/sys/android/inbox.rs | 195 ++++++-------- crates/robius-sms/src/sys/android/mod.rs | 1 + crates/robius-sms/src/sys/android/thread.rs | 150 ++++------- 4 files changed, 402 insertions(+), 217 deletions(-) create mode 100644 crates/robius-sms/src/sys/android/cursor.rs diff --git a/crates/robius-sms/src/sys/android/cursor.rs b/crates/robius-sms/src/sys/android/cursor.rs new file mode 100644 index 0000000..05649d7 --- /dev/null +++ b/crates/robius-sms/src/sys/android/cursor.rs @@ -0,0 +1,273 @@ +// Shared Cursor handling for the Android content-provider reads. +// +// inbox.rs and thread.rs each carried a byte-identical copy of +// get_column_index / is_null_at / get_i64_column / get_i32_column / +// get_string_column (~75 duplicated lines). Both copies had the same +// three defects, which is what duplication does. +// +// A1 -- local reference table overflow. +// +// Every get_*_column called env.new_string(name) to look the column +// index up by name, and getString returned another local ref. That is +// ~10 local references per message row, and JNI local refs are NOT +// freed when the Rust value drops -- they live until the native +// method returns to the JVM. Here that is the whole cursor loop. +// +// ART's default local reference capacity is 512 entries. At ~10 refs +// per row the table overflows at roughly 45-50 messages, and overflow +// is a hard abort ("JNI ERROR (app bug): local reference table +// overflow"), not an Err. Any real inbox has hundreds to thousands of +// messages, so this was very likely the single largest source of +// field crashes in this crate. +// +// Fixed two ways, both needed: +// - Column indices are resolved ONCE per query into a ColumnIndices +// struct, instead of per row per column. That removes the +// new_string churn entirely. +// - Each row is read inside env.with_local_frame(...), so whatever +// the row does allocate is released when the row finishes. +// +// A2 -- pending exceptions were never cleared. +// +// My assessment claimed this crate performed no exception checking at +// all. That was wrong: jni-rs 0.21 expands every checked call through +// `check_exception!`, which calls ExceptionCheck and returns +// Err(Error::JavaException) if one is pending. +// +// The real defect is what happens next. jni-rs does NOT clear the +// exception -- ExceptionClear appears in exactly one place in the +// crate, the public exception_clear(), which nothing here called. So +// the Err propagates while the exception stays pending on the thread, +// and the NEXT JNI call made on that thread aborts the process. +// +// In practice: a SecurityException from a revoked READ_SMS +// permission, or an IllegalStateException from a closed cursor, +// returned a tidy-looking Err -- and then killed the app on the next +// unrelated JNI call, far from the cause. +// +// `clear_pending_exception` below turns a pending exception into a +// plain Err with the exception described to logcat and the thread +// left clean. +// +// Cursor leak -- `?` inside the loop skipped the close() call at the +// end of the function, leaking the Cursor and its CursorWindow +// (typically a 2 MB ashmem region) on every error. CursorGuard closes +// it on drop, on every path. + +use jni::{ + objects::{JObject, JString, JValueGen}, + JNIEnv, +}; + +use crate::Result; + +/// Clear any pending JNI exception, describing it to logcat first. +/// +/// Must be called on every error path that crosses back out of a JNI +/// call, because jni-rs reports the exception but leaves it pending. +pub(crate) fn clear_pending_exception(env: &mut JNIEnv<'_>) { + // Deliberately tolerant: this runs while already handling an error, + // and nothing here may itself return one. + if let Ok(true) = env.exception_check() { + // Sends the stack trace to logcat, which is the only diagnostic + // available once the exception object is discarded. + let _ = env.exception_describe(); + let _ = env.exception_clear(); + } +} + +/// Run `f`, and if it fails make sure the thread is left with no +/// pending exception. +pub(crate) fn guarding_exceptions( + env: &mut JNIEnv<'_>, + f: impl FnOnce(&mut JNIEnv<'_>) -> Result, +) -> Result { + match f(env) { + Ok(v) => Ok(v), + Err(e) => { + clear_pending_exception(env); + Err(e) + } + } +} + +/// Closes an Android Cursor on drop. +/// +/// The previous code called close() only on the success path, so any +/// `?` inside the read loop leaked the cursor. +pub(crate) struct CursorGuard<'local> { + cursor: JObject<'local>, +} + +impl<'local> CursorGuard<'local> { + pub(crate) fn new(cursor: JObject<'local>) -> Self { + Self { cursor } + } + + pub(crate) fn as_obj(&self) -> &JObject<'local> { + &self.cursor + } + + /// Close explicitly, reporting failure. `drop` still runs and is a + /// no-op-if-already-closed on Android. + pub(crate) fn close(&self, env: &mut JNIEnv<'_>) { + if self.cursor.is_null() { + return; + } + if env + .call_method(&self.cursor, "close", "()V", &[]) + .is_err() + { + clear_pending_exception(env); + } + } +} + +/// Column indices resolved once per query. +/// +/// Cursor.getColumnIndex takes a String, so resolving per row per +/// column meant allocating one Java String per column per row. Doing it +/// once turns ~10 allocations per row into ~8 for the entire query. +#[derive(Debug, Default, Clone, Copy)] +pub(crate) struct ColumnIndex(Option); + +impl ColumnIndex { + pub(crate) fn resolve(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result { + let jname = env.new_string(name)?; + let idx = env + .call_method( + cursor, + "getColumnIndex", + "(Ljava/lang/String;)I", + &[JValueGen::Object(&JObject::from(jname))], + )? + .i()?; + Ok(Self(if idx < 0 { None } else { Some(idx) })) + } + + fn get(self) -> Option { + self.0 + } +} + +fn is_null_at(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, idx: i32) -> Result { + env.call_method(cursor, "isNull", "(I)Z", &[JValueGen::Int(idx)])? + .z() + .map_err(Into::into) +} + +pub(crate) fn get_i64( + env: &mut JNIEnv<'_>, + cursor: &JObject<'_>, + col: ColumnIndex, +) -> Result> { + let Some(idx) = col.get() else { return Ok(None) }; + if is_null_at(env, cursor, idx)? { + return Ok(None); + } + Ok(Some( + env.call_method(cursor, "getLong", "(I)J", &[JValueGen::Int(idx)])? + .j()?, + )) +} + +pub(crate) fn get_i32( + env: &mut JNIEnv<'_>, + cursor: &JObject<'_>, + col: ColumnIndex, +) -> Result> { + let Some(idx) = col.get() else { return Ok(None) }; + if is_null_at(env, cursor, idx)? { + return Ok(None); + } + Ok(Some( + env.call_method(cursor, "getInt", "(I)I", &[JValueGen::Int(idx)])? + .i()?, + )) +} + +pub(crate) fn get_bool( + env: &mut JNIEnv<'_>, + cursor: &JObject<'_>, + col: ColumnIndex, +) -> Result> { + Ok(get_i32(env, cursor, col)?.map(|v| v != 0)) +} + +pub(crate) fn get_string( + env: &mut JNIEnv<'_>, + cursor: &JObject<'_>, + col: ColumnIndex, +) -> Result> { + let Some(idx) = col.get() else { return Ok(None) }; + if is_null_at(env, cursor, idx)? { + return Ok(None); + } + + let obj = env + .call_method( + cursor, + "getString", + "(I)Ljava/lang/String;", + &[JValueGen::Int(idx)], + )? + .l()?; + + if obj.is_null() { + return Ok(None); + } + + // AutoLocal releases this reference at end of scope rather than at + // end of the enclosing native call. + let obj = env.auto_local(obj); + let jstr = JString::from(unsafe { JObject::from_raw(obj.as_raw()) }); + let value: String = env.get_string(&jstr)?.into(); + Ok(Some(value)) +} + +/// Local reference frame capacity for reading one row. +/// +/// A row needs at most a handful of references (the String returns). +/// 16 is generous; the point is that the frame is popped per row rather +/// than growing for the whole result set. +pub(crate) const ROW_FRAME_CAPACITY: i32 = 16; + +/// Iterate a cursor, reading each row inside its own local frame. +/// +/// This is the A1 fix: without the per-row frame, references accumulate +/// across the entire result set and ART aborts at ~512. +pub(crate) fn for_each_row( + env: &mut JNIEnv<'_>, + guard: &CursorGuard<'_>, + mut read_row: impl FnMut(&mut JNIEnv<'_>) -> Result, +) -> Result> { + let mut out = Vec::new(); + + loop { + let has_next = match env.call_method(guard.as_obj(), "moveToNext", "()Z", &[]) { + Ok(v) => v.z()?, + Err(e) => { + clear_pending_exception(env); + guard.close(env); + return Err(e.into()); + } + }; + if !has_next { + break; + } + + let row = env.with_local_frame(ROW_FRAME_CAPACITY, |env| read_row(env)); + + match row { + Ok(v) => out.push(v), + Err(e) => { + clear_pending_exception(env); + guard.close(env); + return Err(e); + } + } + } + + guard.close(env); + Ok(out) +} diff --git a/crates/robius-sms/src/sys/android/inbox.rs b/crates/robius-sms/src/sys/android/inbox.rs index defb820..97245e9 100644 --- a/crates/robius-sms/src/sys/android/inbox.rs +++ b/crates/robius-sms/src/sys/android/inbox.rs @@ -1,13 +1,16 @@ use jni::{ - objects::{JObject, JString, JValueGen}, + objects::{JObject, JValueGen}, JNIEnv, }; +use super::cursor::{ + self, ColumnIndex, CursorGuard, clear_pending_exception, guarding_exceptions, +}; use crate::{Error, MessageKind, Result, SmsMessage}; pub(crate) fn list_messages(kind: Option) -> Result> { robius_android_env::with_activity(|env, activity| { - list_messages_inner(env, activity, kind, None) + guarding_exceptions(env, |env| list_messages_inner(env, activity, kind, None)) }) .map_err(|_| Error::AndroidEnvironment) .and_then(|x| x) @@ -15,12 +18,45 @@ pub(crate) fn list_messages(kind: Option) -> Result pub(crate) fn list_thread_messages(thread_id: i64) -> Result> { robius_android_env::with_activity(|env, activity| { - list_messages_inner(env, activity, None, Some(thread_id)) + guarding_exceptions(env, |env| { + list_messages_inner(env, activity, None, Some(thread_id)) + }) }) .map_err(|_| Error::AndroidEnvironment) .and_then(|x| x) } +/// Column indices for the SMS provider, resolved once per query. +/// +/// Previously each of these was looked up by name for every row, which +/// allocated a Java String per column per row and was the bulk of the +/// local-reference pressure behind A1. +struct SmsColumns { + id: ColumnIndex, + thread_id: ColumnIndex, + address: ColumnIndex, + body: ColumnIndex, + date: ColumnIndex, + date_sent: ColumnIndex, + read: ColumnIndex, + kind: ColumnIndex, +} + +impl SmsColumns { + fn resolve(env: &mut JNIEnv<'_>, cursor: &JObject<'_>) -> Result { + Ok(Self { + id: ColumnIndex::resolve(env, cursor, "_id")?, + thread_id: ColumnIndex::resolve(env, cursor, "thread_id")?, + address: ColumnIndex::resolve(env, cursor, "address")?, + body: ColumnIndex::resolve(env, cursor, "body")?, + date: ColumnIndex::resolve(env, cursor, "date")?, + date_sent: ColumnIndex::resolve(env, cursor, "date_sent")?, + read: ColumnIndex::resolve(env, cursor, "read")?, + kind: ColumnIndex::resolve(env, cursor, "type")?, + }) + } +} + fn list_messages_inner( env: &mut JNIEnv<'_>, activity: &JObject<'_>, @@ -87,15 +123,40 @@ fn list_messages_inner( return Ok(Vec::new()); } - let mut messages = Vec::new(); + // From here on the cursor is closed on every path, including `?`. + let guard = CursorGuard::new(cursor); - while env.call_method(&cursor, "moveToNext", "()Z", &[])?.z()? { - messages.push(read_message_from_cursor(env, &cursor)?); - } + let columns = match SmsColumns::resolve(env, guard.as_obj()) { + Ok(c) => c, + Err(e) => { + clear_pending_exception(env); + guard.close(env); + return Err(e); + } + }; - let _ = env.call_method(&cursor, "close", "()V", &[]); + cursor::for_each_row(env, &guard, |env| { + read_message(env, guard.as_obj(), &columns) + }) +} - Ok(messages) +fn read_message( + env: &mut JNIEnv<'_>, + cursor: &JObject<'_>, + columns: &SmsColumns, +) -> Result { + Ok(SmsMessage { + id: cursor::get_i64(env, cursor, columns.id)?.unwrap_or_default(), + thread_id: cursor::get_i64(env, cursor, columns.thread_id)?, + address: cursor::get_string(env, cursor, columns.address)?, + body: cursor::get_string(env, cursor, columns.body)?, + date_ms: cursor::get_i64(env, cursor, columns.date)?, + date_sent_ms: cursor::get_i64(env, cursor, columns.date_sent)?, + read: cursor::get_bool(env, cursor, columns.read)?, + kind: cursor::get_i32(env, cursor, columns.kind)? + .map(message_kind_from_android_type) + .unwrap_or(MessageKind::Unknown(-1)), + }) } fn uri_for_kind(kind: Option) -> &'static str { @@ -111,119 +172,7 @@ fn uri_for_kind(kind: Option) -> &'static str { } } -fn read_message_from_cursor(env: &mut JNIEnv<'_>, cursor: &JObject<'_>) -> Result { - let id = get_i64_column(env, cursor, "_id")?.unwrap_or_default(); - let thread_id = get_i64_column(env, cursor, "thread_id")?; - let address = get_string_column(env, cursor, "address")?; - let body = get_string_column(env, cursor, "body")?; - let date_ms = get_i64_column(env, cursor, "date")?; - let date_sent_ms = get_i64_column(env, cursor, "date_sent")?; - let read = get_bool_column(env, cursor, "read")?; - let kind = get_i32_column(env, cursor, "type")? - .map(message_kind_from_android_type) - .unwrap_or(MessageKind::Unknown(-1)); - - Ok(SmsMessage { - id, - thread_id, - address, - body, - date_ms, - date_sent_ms, - read, - kind, - }) -} - -fn get_column_index(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let jname = env.new_string(name)?; - let idx = env - .call_method( - cursor, - "getColumnIndex", - "(Ljava/lang/String;)I", - &[JValueGen::Object(&JObject::from(jname))], - )? - .i()?; - - if idx < 0 { - Ok(None) - } else { - Ok(Some(idx)) - } -} - -fn is_null_at(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, idx: i32) -> Result { - env.call_method(cursor, "isNull", "(I)Z", &[JValueGen::Int(idx)])? - .z() - .map_err(|e| e.into()) -} - -fn get_i64_column(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - Ok(Some( - env.call_method(cursor, "getLong", "(I)J", &[JValueGen::Int(idx)])? - .j()?, - )) -} - -fn get_i32_column(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - Ok(Some( - env.call_method(cursor, "getInt", "(I)I", &[JValueGen::Int(idx)])? - .i()?, - )) -} - -fn get_bool_column( - env: &mut JNIEnv<'_>, - cursor: &JObject<'_>, - name: &str, -) -> Result> { - Ok(get_i32_column(env, cursor, name)?.map(|v| v != 0)) -} - -fn get_string_column( - env: &mut JNIEnv<'_>, - cursor: &JObject<'_>, - name: &str, -) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - - let obj = env - .call_method( - cursor, - "getString", - "(I)Ljava/lang/String;", - &[JValueGen::Int(idx)], - )? - .l()?; - - if obj.is_null() { - return Ok(None); - } - - let jstr = JString::from(obj); - let string_val: String = env.get_string(&jstr)?.into(); - Ok(Some(string_val)) -} - -fn message_kind_from_android_type(value: i32) -> MessageKind { +pub(crate) fn message_kind_from_android_type(value: i32) -> MessageKind { match value { 1 => MessageKind::Inbox, 2 => MessageKind::Sent, @@ -233,4 +182,4 @@ fn message_kind_from_android_type(value: i32) -> MessageKind { 6 => MessageKind::Queued, other => MessageKind::Unknown(other), } -} \ No newline at end of file +} diff --git a/crates/robius-sms/src/sys/android/mod.rs b/crates/robius-sms/src/sys/android/mod.rs index 5d59e9e..5c10c2d 100644 --- a/crates/robius-sms/src/sys/android/mod.rs +++ b/crates/robius-sms/src/sys/android/mod.rs @@ -1,3 +1,4 @@ +mod cursor; mod inbox; mod permissions; mod receivers; // Fixed: was receive diff --git a/crates/robius-sms/src/sys/android/thread.rs b/crates/robius-sms/src/sys/android/thread.rs index 31d16b3..22c74b2 100644 --- a/crates/robius-sms/src/sys/android/thread.rs +++ b/crates/robius-sms/src/sys/android/thread.rs @@ -1,14 +1,42 @@ use jni::{ - objects::{JObject, JString, JValueGen}, + objects::{JObject, JValueGen}, JNIEnv, }; +use super::cursor::{ + self, ColumnIndex, CursorGuard, clear_pending_exception, guarding_exceptions, +}; use crate::{Error, Result, SmsThread}; pub(crate) fn list_threads() -> Result> { - robius_android_env::with_activity(list_threads_inner) - .map_err(|_| Error::AndroidEnvironment) - .and_then(|x| x) + robius_android_env::with_activity(|env, activity| { + guarding_exceptions(env, |env| list_threads_inner(env, activity)) + }) + .map_err(|_| Error::AndroidEnvironment) + .and_then(|x| x) +} + +/// Conversation columns, resolved once per query rather than per row. +struct ThreadColumns { + thread_id: ColumnIndex, + row_id: ColumnIndex, + snippet: ColumnIndex, + msg_count: ColumnIndex, + recipient_ids: ColumnIndex, + date: ColumnIndex, +} + +impl ThreadColumns { + fn resolve(env: &mut JNIEnv<'_>, cursor: &JObject<'_>) -> Result { + Ok(Self { + thread_id: ColumnIndex::resolve(env, cursor, "thread_id")?, + row_id: ColumnIndex::resolve(env, cursor, "_id")?, + snippet: ColumnIndex::resolve(env, cursor, "snippet")?, + msg_count: ColumnIndex::resolve(env, cursor, "msg_count")?, + recipient_ids: ColumnIndex::resolve(env, cursor, "recipient_ids")?, + date: ColumnIndex::resolve(env, cursor, "date")?, + }) + } } fn list_threads_inner(env: &mut JNIEnv<'_>, activity: &JObject<'_>) -> Result> { @@ -52,98 +80,32 @@ fn list_threads_inner(env: &mut JNIEnv<'_>, activity: &JObject<'_>) -> Result c, + Err(e) => { + clear_pending_exception(env); + guard.close(env); + return Err(e); + } + }; + + cursor::for_each_row(env, &guard, |env| { + let obj = guard.as_obj(); + + // The simple conversations view exposes _id; the full view + // exposes thread_id. Prefer thread_id, fall back to _id. + let thread_id = cursor::get_i64(env, obj, columns.thread_id)? + .or(cursor::get_i64(env, obj, columns.row_id)?) .unwrap_or_default(); - threads.push(SmsThread { + Ok(SmsThread { thread_id, - snippet: get_string_column(env, &cursor, "snippet")?, - message_count: get_i32_column(env, &cursor, "msg_count")?, - recipient_ids: get_string_column(env, &cursor, "recipient_ids")?, - date_ms: get_i64_column(env, &cursor, "date")?, - }); - } - - let _ = env.call_method(&cursor, "close", "()V", &[]); - - Ok(threads) + snippet: cursor::get_string(env, obj, columns.snippet)?, + message_count: cursor::get_i32(env, obj, columns.msg_count)?, + recipient_ids: cursor::get_string(env, obj, columns.recipient_ids)?, + date_ms: cursor::get_i64(env, obj, columns.date)?, + }) + }) } - -fn get_column_index(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let jname = env.new_string(name)?; - let idx = env - .call_method( - cursor, - "getColumnIndex", - "(Ljava/lang/String;)I", - &[JValueGen::Object(&JObject::from(jname))], - )? - .i()?; - if idx < 0 { - Ok(None) - } else { - Ok(Some(idx)) - } -} - -fn is_null_at(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, idx: i32) -> Result { - env.call_method(cursor, "isNull", "(I)Z", &[JValueGen::Int(idx)])? - .z() - .map_err(|e| e.into()) -} - -fn get_i64_column(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - Ok(Some( - env.call_method(cursor, "getLong", "(I)J", &[JValueGen::Int(idx)])? - .j()?, - )) -} - -fn get_i32_column(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - Ok(Some( - env.call_method(cursor, "getInt", "(I)I", &[JValueGen::Int(idx)])? - .i()?, - )) -} - -fn get_string_column(env: &mut JNIEnv<'_>, cursor: &JObject<'_>, name: &str) -> Result> { - let Some(idx) = get_column_index(env, cursor, name)? else { - return Ok(None); - }; - if is_null_at(env, cursor, idx)? { - return Ok(None); - } - - let obj = env - .call_method( - cursor, - "getString", - "(I)Ljava/lang/String;", - &[JValueGen::Int(idx)], - )? - .l()?; - - if obj.is_null() { - return Ok(None); - } - - let jstr = JString::from(obj); - let string_val: String = env.get_string(&jstr)?.into(); - Ok(Some(string_val)) -} \ No newline at end of file From 0ed64c0435495f0b075475c872bcc1616a8a2b23 Mon Sep 17 00:00:00 2001 From: nigig-ci Date: Fri, 31 Jul 2026 22:22:08 +0000 Subject: [PATCH 2/2] fix(sms): send long messages whole, and confirm before bulk (A5, A7, A8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A5 -- long messages were silently truncated. send_text_message() called SmsManager.sendTextMessage unconditionally. That API is only defined for a body that fits one PDU: 160 GSM-7 characters, or 70 once anything forces UCS-2. Past that the behaviour is carrier- and OEM-dependent -- silent truncation, silent failure, or an exception -- and the UI reported "Sent" regardless, because a JNI call returning cleanly only means the call was made. Now uses divideMessage() + sendMultipartTextMessage() when the body needs more than one part, so the recipient's handset reassembles it. Adds segment_count(), which computes encoding and segment count on any host so it is unit-testable and usable by UI. Correct GSM-7 modelling matters here and is not intuitive: - the extended characters ^ { } \ [ ] ~ | € cost TWO septets each - concatenation costs 7 septets per part, so the limit drops from 160 to 153 (and 70 to 67 for UCS-2) - non-BMP characters such as emoji are surrogate pairs and cost two UTF-16 units - e/a/o/n/u with diacritics ARE in GSM-7. My first version of the boundary test assumed é forced UCS-2 and failed; Swahili, French and German text still bills at the 160-char rate. Arabic and emoji do not. A7 -- bulk send had no confirmation. One tap on "Send SMS to Selected", one tap on Send, and N real, billed, irreversible messages went out. For a feature whose purpose is mass-messaging a scraped business directory, that is a financial-harm defect rather than a UX gap. Send now arms a confirmation quoting SEGMENTS, not messages, because segments are what the user pays for and the number is surprising: one emoji in the body forces UCS-2 and can turn "200 messages" into 800 paid segments. Editing the body or the recipient list re-arms the prompt, so a confirmation cannot be inherited by different content. A8 -- SCHEDULE_EXACT_ALARM was neither requested nor documented. Android 12+ refuses exact alarms without it, and it is a special-access permission: a runtime prompt cannot grant it, the user must enable "Alarms & reminders" in Settings. Without it scheduled sends are subject to Doze batching. Documented in the crate docs and README, along with two things that were not written down anywhere: Ok from send_sms means "handed to the platform", NOT delivered (both PendingIntents are null, so nothing can report back); and callers are billed per segment. Verified: robius-sms 15 tests pass (8 new for segmentation), nigig-sms 15 pass, clippy -D warnings clean on host and aarch64-linux-android, nigig-sms clippy ratchet holds at 50, gates and supply-chain green. --- .../src/sms_frame/bulk/sms_bulk_page.rs | 56 ++++- .../sms_frame/schedule/sms_schedule_page.rs | 4 + crates/robius-sms/README.md | 27 ++- crates/robius-sms/src/lib.rs | 192 ++++++++++++++++++ crates/robius-sms/src/sys/android/send.rs | 56 ++++- 5 files changed, 330 insertions(+), 5 deletions(-) diff --git a/crates/apps/nigig-sms/src/sms_frame/bulk/sms_bulk_page.rs b/crates/apps/nigig-sms/src/sms_frame/bulk/sms_bulk_page.rs index 9517322..a3abed6 100644 --- a/crates/apps/nigig-sms/src/sms_frame/bulk/sms_bulk_page.rs +++ b/crates/apps/nigig-sms/src/sms_frame/bulk/sms_bulk_page.rs @@ -272,6 +272,12 @@ pub struct SmsBulkPage { active_industry: String, #[rust] tabs_initialized: bool, + /// A7: the (body, recipient_count) a confirmation prompt was shown + /// for. The next Send tap only dispatches if it still matches, so + /// editing the body or the recipient list re-arms the prompt rather + /// than silently sending the new content. + #[rust] + pending_bulk_send: Option<(String, usize)>, } impl ScriptHook for SmsBulkPage { @@ -562,18 +568,66 @@ impl SmsBulkPage { let trimmed_body = body.trim(); if trimmed_body.is_empty() { self.view.label(cx, ids!(bulk_status)).set_text(cx, "Please type a message."); + self.pending_bulk_send = None; self.view.redraw(cx); return; } let recipients: Vec = recipients_text.lines().map(|l| l.trim().to_string()).filter(|l| !l.is_empty()).collect(); if recipients.is_empty() { self.view.label(cx, ids!(bulk_status)).set_text(cx, "Please enter at least one recipient."); + self.pending_bulk_send = None; self.view.redraw(cx); return; } + + // A7: require an explicit second tap before spending money. + // + // One tap on "Send SMS to Selected" followed by one tap here + // used to dispatch N real, billed, irreversible messages with no + // confirmation of any kind -- for a feature whose whole purpose + // is mass-messaging a scraped business directory. + // + // The prompt quotes SEGMENTS, not messages, because that is what + // the user is billed for and the number is not intuitive: one + // emoji forces the whole body to UCS-2 and drops the per-part + // limit from 160 characters to 70, so "one message" to 200 + // companies can silently be 800 paid segments. + let seg = robius_sms::segment_count(trimmed_body); + let total = recipients.len(); + let total_segments = seg.segments.saturating_mul(total); + + let confirmation_matches = self + .pending_bulk_send + .as_ref() + .is_some_and(|(b, n)| b == trimmed_body && *n == total); + + if !confirmation_matches { + self.pending_bulk_send = Some((trimmed_body.to_string(), total)); + let encoding = match seg.encoding { + robius_sms::SmsEncoding::Gsm7 => "GSM-7", + robius_sms::SmsEncoding::Ucs2 => "Unicode", + }; + self.view.label(cx, ids!(bulk_status)).set_text( + cx, + &format!( + "Send {} SMS ({} {} segment{} each, {} total)? \ + This cannot be undone. Tap Send again to confirm.", + total, + seg.segments, + encoding, + if seg.segments == 1 { "" } else { "s" }, + total_segments, + ), + ); + self.view.redraw(cx); + return; + } + + // Confirmed: clear the arming state so a later tap re-prompts. + self.pending_bulk_send = None; + let mut sent = 0; let mut failed = 0; - let total = recipients.len(); for recipient in &recipients { match send_single(recipient, trimmed_body) { Ok(()) => sent += 1, diff --git a/crates/apps/nigig-sms/src/sms_frame/schedule/sms_schedule_page.rs b/crates/apps/nigig-sms/src/sms_frame/schedule/sms_schedule_page.rs index 06523dd..6babcae 100644 --- a/crates/apps/nigig-sms/src/sms_frame/schedule/sms_schedule_page.rs +++ b/crates/apps/nigig-sms/src/sms_frame/schedule/sms_schedule_page.rs @@ -17,6 +17,10 @@ use makepad_widgets::*; /// across app restarts, not just within one run. A read/write failure /// falls back to a timestamp-derived id: still unique in practice, and /// far better than handing back a known-colliding constant. +/// +/// Only reachable on Android: schedule_sms() is a no-op stub elsewhere, +/// so on other targets this would be dead code. +#[cfg(target_os = "android")] fn next_schedule_id() -> i32 { use std::sync::Mutex; use crate::chats::sms_frame::lock_ext::LockRecover; diff --git a/crates/robius-sms/README.md b/crates/robius-sms/README.md index b69aebb..6b98079 100644 --- a/crates/robius-sms/README.md +++ b/crates/robius-sms/README.md @@ -24,6 +24,12 @@ Add these to your `AndroidManifest.xml`: + + + //! //! +//! +//! +//! //! //! //! //! ``` +//! +//! ## Delivery is not confirmed +//! +//! [`send_sms`] returning `Ok` means the send was handed to the +//! platform, NOT that the message was delivered or even transmitted. +//! This crate passes null for both the `sentIntent` and `deliveryIntent` +//! PendingIntents, so there is nothing to report back. Do not present +//! `Ok` to a user as "delivered". +//! +//! ## Cost +//! +//! Callers are billed per SEGMENT. Use [`segment_count`] before sending +//! to find out what a body actually costs: any character outside the +//! GSM-7 alphabet -- one emoji is enough -- forces the whole message to +//! UCS-2 and drops the per-part limit from 160 characters to 70. mod error; mod sys; @@ -111,6 +135,95 @@ pub struct ScheduledMessage { pub interval_ms: Option, } +/// How a body will be encoded on the air interface. +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum SmsEncoding { + /// The GSM 7-bit default alphabet: 160 chars in one part, 153 per + /// part when concatenated (7 septets go to the segmentation header). + Gsm7, + /// UCS-2, forced by any character outside GSM-7 -- one emoji is + /// enough. 70 chars in one part, 67 per part when concatenated. + Ucs2, +} + +/// What sending `body` will actually cost. +/// +/// Callers are billed per SEGMENT, not per message, and the counts drop +/// sharply the moment a body leaves GSM-7. Surfacing this before a send +/// is the difference between "send 200 messages" and "send 800". +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub struct SmsSegments { + pub encoding: SmsEncoding, + /// Number of PDUs that will be sent. Zero only for an empty body. + pub segments: usize, + /// Characters that still fit in the final segment. + pub remaining_in_last: usize, +} + +/// Characters in the GSM 7-bit default alphabet that occupy two septets. +const GSM7_EXTENDED: &[char] = &['^', '{', '}', '\\', '[', ']', '~', '|', '€']; + +fn gsm7_septets(c: char) -> Option { + const GSM7_BASIC: &str = "@£$¥èéùìòÇØøÅåΔ_ΦΓΛΩΠΨΣΘΞÆæßÉ !\"#¤%&'()*+,-./0123456789:;<=>?\ +¡ABCDEFGHIJKLMNOPQRSTUVWXYZÄÖÑܧ¿abcdefghijklmnopqrstuvwxyzäöñüà"; + if GSM7_EXTENDED.contains(&c) { + // Escape sequence: ESC + the character. + Some(2) + } else if c == '\n' || c == '\r' || GSM7_BASIC.contains(c) { + Some(1) + } else { + None + } +} + +/// Work out the encoding and segment count for a body. +/// +/// Mirrors what `SmsManager.divideMessage` does on the device, but runs +/// anywhere, so it is unit-testable and usable by UI that must warn +/// before spending the user's money. +pub fn segment_count(body: &str) -> SmsSegments { + // Any character outside GSM-7 forces the whole message to UCS-2. + let mut septets = 0usize; + let mut gsm7 = true; + for c in body.chars() { + match gsm7_septets(c) { + Some(n) => septets += n, + None => { + gsm7 = false; + break; + } + } + } + + let (encoding, units, single, multi) = if gsm7 { + (SmsEncoding::Gsm7, septets, 160usize, 153usize) + } else { + // UCS-2 counts UTF-16 code units, so a non-BMP character such as + // an emoji costs two. + let units = body.chars().map(|c| c.len_utf16()).sum::(); + (SmsEncoding::Ucs2, units, 70usize, 67usize) + }; + + if units == 0 { + return SmsSegments { encoding, segments: 0, remaining_in_last: single }; + } + if units <= single { + return SmsSegments { + encoding, + segments: 1, + remaining_in_last: single - units, + }; + } + + let segments = units.div_ceil(multi); + let used_in_last = units - multi * (segments - 1); + SmsSegments { + encoding, + segments, + remaining_in_last: multi - used_in_last, + } +} + impl ScheduleRequest { /// Validate a schedule request. /// @@ -413,4 +526,83 @@ mod tests { assert!(r.validate().is_err()); } } + + // ── A5: multipart segmentation ────────────────────────────────── + + #[test] + fn empty_body_is_zero_segments() { + assert_eq!(segment_count("").segments, 0); + } + + #[test] + fn short_ascii_is_one_gsm7_segment() { + let s = segment_count("Hello"); + assert_eq!(s.encoding, SmsEncoding::Gsm7); + assert_eq!(s.segments, 1); + assert_eq!(s.remaining_in_last, 155); + } + + #[test] + fn gsm7_boundary_is_160_then_2_parts() { + assert_eq!(segment_count(&"a".repeat(160)).segments, 1); + // 161 chars no longer fit one PDU: concatenation costs 7 + // septets per part, so the limit drops to 153. + assert_eq!(segment_count(&"a".repeat(161)).segments, 2); + assert_eq!(segment_count(&"a".repeat(306)).segments, 2); + assert_eq!(segment_count(&"a".repeat(307)).segments, 3); + } + + #[test] + fn a_single_emoji_forces_ucs2_and_collapses_the_limit() { + // The core of A5. This body is 100 characters -- comfortably + // "one SMS" by the old byte/char intuition -- but one emoji + // forces UCS-2, where the single-part limit is 70. + let body = format!("{}🎂", "a".repeat(99)); + let s = segment_count(&body); + assert_eq!(s.encoding, SmsEncoding::Ucs2); + assert!(s.segments > 1, "expected multipart, got {s:?}"); + } + + #[test] + fn ucs2_boundary_is_70_then_2_parts() { + // NOTE: é IS in the GSM-7 alphabet (as are à, ä, ö, ñ, ü, Å...), + // so it does NOT force UCS-2 -- my first version of this test + // asserted it did and failed. Swahili and most Western European + // text therefore still bills at the 160-char rate. Use Arabic, + // which genuinely has no GSM-7 representation. + assert_eq!(segment_count(&"é".repeat(71)).encoding, SmsEncoding::Gsm7); + + assert_eq!(segment_count(&"م".repeat(70)).encoding, SmsEncoding::Ucs2); + assert_eq!(segment_count(&"م".repeat(70)).segments, 1); + assert_eq!(segment_count(&"م".repeat(71)).segments, 2); + } + + #[test] + fn emoji_costs_two_utf16_units() { + // Non-BMP characters are surrogate pairs in UCS-2, so 35 emoji + // exactly fill one 70-unit segment. + assert_eq!(segment_count(&"🎂".repeat(35)).segments, 1); + assert_eq!(segment_count(&"🎂".repeat(36)).segments, 2); + } + + #[test] + fn gsm7_extended_characters_cost_two_septets() { + // { } [ ] ~ | ^ \\ and € take an escape septet each. + assert_eq!(segment_count(&"{".repeat(80)).segments, 1); + assert_eq!(segment_count(&"{".repeat(81)).segments, 2); + assert_eq!(segment_count(&"{".repeat(80)).encoding, SmsEncoding::Gsm7); + } + + #[test] + fn a_realistic_bulk_body_reports_its_true_cost() { + // What the bulk sender blasts to every selected company. The UI + // must be able to say "this is N messages each" BEFORE sending. + let body = "Dear customer, our new branch opens Monday. Visit us for 20% off all items this week only. Reply STOP to opt out."; + let s = segment_count(body); + assert_eq!(s.encoding, SmsEncoding::Gsm7); + assert!(s.segments >= 1); + // Adding one emoji to the same copy multiplies the bill. + let with_emoji = format!("{body} 🎉"); + assert!(segment_count(&with_emoji).segments > s.segments); + } } diff --git a/crates/robius-sms/src/sys/android/send.rs b/crates/robius-sms/src/sys/android/send.rs index 2a9b9b9..86cd26f 100644 --- a/crates/robius-sms/src/sys/android/send.rs +++ b/crates/robius-sms/src/sys/android/send.rs @@ -25,6 +25,20 @@ pub(crate) fn send_bulk_sms(request: &BulkSendRequest) -> Result<()> { .and_then(|x| x) } +/// Send one SMS, splitting it into multiple parts if it does not fit in +/// a single PDU (bug A5). +/// +/// `SmsManager.sendTextMessage` is only defined for a body that fits one +/// message: 160 GSM-7 characters, or 70 if the body contains any +/// character outside GSM-7 (which forces UCS-2 -- a single emoji is +/// enough). Past that the behaviour is carrier- and OEM-dependent: +/// silent truncation, silent failure, or an exception. The crate called +/// it unconditionally, and the UI reported success either way, because +/// a JNI call returning cleanly only means the call was made. +/// +/// `divideMessage` splits the body the way the platform intends, and +/// `sendMultipartTextMessage` sends the parts as a linked sequence that +/// the recipient's handset reassembles. pub(crate) fn send_text_message(env: &mut JNIEnv<'_>, recipient: &str, body: &str) -> Result<()> { let sms_manager = env .call_static_method( @@ -38,14 +52,50 @@ pub(crate) fn send_text_message(env: &mut JNIEnv<'_>, recipient: &str, body: &st let destination = env.new_string(recipient)?; let text = env.new_string(body)?; + // ArrayList divideMessage(String text) + let parts = env + .call_method( + &sms_manager, + "divideMessage", + "(Ljava/lang/String;)Ljava/util/ArrayList;", + &[JValueGen::Object(&JObject::from(text))], + )? + .l()?; + + let part_count = env.call_method(&parts, "size", "()I", &[])?.i()?; + + if part_count <= 1 { + // Single part: use the simple call. Re-create the string, since + // divideMessage consumed the local ref above. + let text = env.new_string(body)?; + env.call_method( + &sms_manager, + "sendTextMessage", + "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Landroid/app/PendingIntent;Landroid/app/PendingIntent;)V", + &[ + JValueGen::Object(&JObject::from(destination)), + JValueGen::Object(&JObject::null()), + JValueGen::Object(&JObject::from(text)), + JValueGen::Object(&JObject::null()), + JValueGen::Object(&JObject::null()), + ], + )?; + return Ok(()); + } + + // void sendMultipartTextMessage(String destinationAddress, + // String scAddress, + // ArrayList parts, + // ArrayList sentIntents, + // ArrayList deliveryIntents) env.call_method( &sms_manager, - "sendTextMessage", - "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Landroid/app/PendingIntent;Landroid/app/PendingIntent;)V", + "sendMultipartTextMessage", + "(Ljava/lang/String;Ljava/lang/String;Ljava/util/ArrayList;Ljava/util/ArrayList;Ljava/util/ArrayList;)V", &[ JValueGen::Object(&JObject::from(destination)), JValueGen::Object(&JObject::null()), - JValueGen::Object(&JObject::from(text)), + JValueGen::Object(&parts), JValueGen::Object(&JObject::null()), JValueGen::Object(&JObject::null()), ],