JNI: clear out the sqlite3_context native pointer after calling UDF callbacks which do not have an argv (as was already done for those which have an argv). Add related tests and code commentary.

FossilOrigin-Name: 138f40543b26b2e02e27d830d92e30b12cfef5a8dc3f0b58b39c68e1b3c91cc6
2023-11-15 08:29:42 +00:00 · 2023-11-15 08:29:42 +00:00 · bd06d8672d
parent 4d6b10cdbe
commit bd06d8672d
6 changed files with 84 additions and 22 deletions
--- a/ext/jni/README.md
+++ b/ext/jni/README.md
@ -16,9 +16,8 @@ Technical support is available in the forum:
 > **FOREWARNING:** this subproject is very much in development and
  subject to any number of changes. Please do not rely on any
  information about its API until this disclaimer is removed.  The JNI
-  bindings released with version 3.43 are a "tech preview" and 3.44
-  will be "final," at which point strong backward compatibility
-  guarantees will apply.
+  bindings released with version 3.43 are a "tech preview." Once
+  finalized, strong backward compatibility guarantees will apply.

 Project goals/requirements:

@ -43,11 +42,13 @@ Non-goals:
 - Creation of high-level OO wrapper APIs. Clients are free to create
  them off of the C-style API.

+- Virtual tables are unlikely to be supported due to the amount of
+  glue code needed to fit them into Java.
+
 - Support for mixed-mode operation, where client code accesses SQLite
  both via the Java-side API and the C API via their own native
-  code. In such cases, proxy functionalities (primarily callback
-  handler wrappers of all sorts) may fail because the C-side use of
-  the SQLite APIs will bypass those proxies.
+  code. Such cases would be a minefield of potential mis-interactions
+  between this project's JNI bindings and mixed-mode client code.


 Hello World
--- a/ext/jni/src/c/sqlite3-jni.c
+++ b/ext/jni/src/c/sqlite3-jni.c
@ -1995,13 +1995,16 @@ error_oom:

 /*
 ** Requires that jCx and jArgv are sqlite3_context
-** resp. array-of-sqlite3_value values initialized by udf_args(). This
+** resp. array-of-sqlite3_value values initialized by udf_args(). The
+** latter will be 0-and-NULL for UDF types with no arguments. This
 ** function zeroes out the nativePointer member of jCx and each entry
 ** in jArgv. This is a safety-net precaution to avoid undefined
-** behavior if a Java-side UDF holds a reference to one of its
-** arguments. This MUST be called from any function which successfully
-** calls udf_args(), after calling the corresponding UDF and checking
-** its exception status. It MUST NOT be called in any other case.
+** behavior if a Java-side UDF holds a reference to its context or one
+** of its arguments. This MUST be called from any function which
+** successfully calls udf_args(), after calling the corresponding UDF
+** and checking its exception status, or which Java-wraps a
+** sqlite3_context for use with a UDF(ish) call. It MUST NOT be called
+** in any other case.
 */
 static void udf_unargs(JNIEnv *env, jobject jCx, int argc, jobjectArray jArgv){
  int i = 0;
@ -2009,8 +2012,29 @@ static void udf_unargs(JNIEnv *env, jobject jCx, int argc, jobjectArray jArgv){
  NativePointerHolder_set(S3JniNph(sqlite3_context), jCx, 0);
  for( ; i < argc; ++i ){
    jobject jsv = (*env)->GetObjectArrayElement(env, jArgv, i);
+    /*
+    ** There is a potential Java-triggerable case of Undefined
+    ** Behavior here, but it would require intentional misuse of the
+    ** API:
+    **
+    ** If a Java UDF grabs an sqlite3_value from its argv and then
+    ** assigns that element to null, it becomes unreachable to us so
+    ** we cannot clear out its pointer. That Java-side object's
+    ** getNativePointer() will then refer to a stale value, so passing
+    ** it into (e.g.) sqlite3_value_SOMETHING() would invoke UB.
+    **
+    ** High-level wrappers can avoid that possibility if they do not
+    ** expose sqlite3_value directly to clients (as is the case in
+    ** org.sqlite.jni.wrapper1.SqlFunction).
+    **
+    ** One potential (but expensive) workaround for this would be to
+    ** privately store a duplicate argv array in each sqlite3_context
+    ** wrapper object, and clear the native pointers from that copy.
+    */
    assert(jsv && "Someone illegally modified a UDF argument array.");
-    NativePointerHolder_set(S3JniNph(sqlite3_value), jsv, 0);
+    if( jsv ){
+      NativePointerHolder_set(S3JniNph(sqlite3_value), jsv, 0);
+    }
  }
 }

@ -2099,6 +2123,7 @@ static int udf_xFV(sqlite3_context* cx, S3JniUdf * s,
      rc = udf_report_exception(env, isFinal, cx, s->zFuncName,
                                zFuncType);
    }
+    udf_unargs(env, jcx, 0, 0);
    S3JniUnrefLocal(jcx);
  }else{
    if( isFinal ) sqlite3_result_error_nomem(cx);
--- a/ext/jni/src/org/sqlite/jni/capi/Tester1.java
+++ b/ext/jni/src/org/sqlite/jni/capi/Tester1.java
@ -928,15 +928,28 @@ public class Tester1 implements Runnable {
      // To confirm that xFinal() is called with no aggregate state
      // when the corresponding result set is empty.
      new ValueHolder<>(false);
+    final ValueHolder<sqlite3_value[]> neverEverDoThisInClientCode = new ValueHolder<>(null);
+    final ValueHolder<sqlite3_context> neverEverDoThisInClientCode2 = new ValueHolder<>(null);
    SQLFunction func = new AggregateFunction<Integer>(){
        @Override
        public void xStep(sqlite3_context cx, sqlite3_value[] args){
+          if( null==neverEverDoThisInClientCode.value ){
+            /* !!!NEVER!!! hold a reference to an sqlite3_value or
+               sqlite3_context object like this in client code! They
+               are ONLY legal for the duration of their single
+               call. We do it here ONLY to test that the defenses
+               against clients doing this are working. */
+            neverEverDoThisInClientCode.value = args;
+          }
          final ValueHolder<Integer> agg = this.getAggregateState(cx, 0);
          agg.value += sqlite3_value_int(args[0]);
          affirm( agg == this.getAggregateState(cx, 0) );
        }
        @Override
        public void xFinal(sqlite3_context cx){
+          if( null==neverEverDoThisInClientCode2.value ){
+            neverEverDoThisInClientCode2.value = cx;
+          }
          final Integer v = this.takeAggregateState(cx);
          if(null == v){
            xFinalNull.value = true;
@ -961,6 +974,10 @@ public class Tester1 implements Runnable {
    }
    affirm( 1==n );
    affirm(!xFinalNull.value);
+    affirm( null!=neverEverDoThisInClientCode.value );
+    affirm( null!=neverEverDoThisInClientCode2.value );
+    affirm( 0<neverEverDoThisInClientCode.value.length );
+    affirm( 0==neverEverDoThisInClientCode2.value.getNativePointer() );
    sqlite3_reset(stmt);
    affirm( 1==sqlite3_stmt_status(stmt, SQLITE_STMTSTATUS_RUN, false) );
    // Ensure that the accumulator is reset on subsequent calls...
--- a/ext/jni/src/org/sqlite/jni/wrapper1/Sqlite.java
+++ b/ext/jni/src/org/sqlite/jni/wrapper1/Sqlite.java
@ -33,6 +33,7 @@ public final class Sqlite implements AutoCloseable  {
  private static final boolean JNI_SUPPORTS_NIO =
    CApi.sqlite3_jni_supports_nio();

+  // Result codes
  public static final int OK = CApi.SQLITE_OK;
  public static final int ERROR = CApi.SQLITE_ERROR;
  public static final int INTERNAL = CApi.SQLITE_INTERNAL;
@ -138,14 +139,17 @@ public final class Sqlite implements AutoCloseable  {
  public static final int AUTH_USER = CApi.SQLITE_AUTH_USER;
  public static final int OK_LOAD_PERMANENTLY = CApi.SQLITE_OK_LOAD_PERMANENTLY;

+  // sqlite3_open() flags
  public static final int OPEN_READWRITE = CApi.SQLITE_OPEN_READWRITE;
  public static final int OPEN_CREATE = CApi.SQLITE_OPEN_CREATE;
  public static final int OPEN_EXRESCODE = CApi.SQLITE_OPEN_EXRESCODE;

+  // transaction state
  public static final int TXN_NONE = CApi.SQLITE_TXN_NONE;
  public static final int TXN_READ = CApi.SQLITE_TXN_READ;
  public static final int TXN_WRITE = CApi.SQLITE_TXN_WRITE;

+  // sqlite3_status() ops
  public static final int STATUS_MEMORY_USED = CApi.SQLITE_STATUS_MEMORY_USED;
  public static final int STATUS_PAGECACHE_USED = CApi.SQLITE_STATUS_PAGECACHE_USED;
  public static final int STATUS_PAGECACHE_OVERFLOW = CApi.SQLITE_STATUS_PAGECACHE_OVERFLOW;
@ -154,6 +158,7 @@ public final class Sqlite implements AutoCloseable  {
  public static final int STATUS_PAGECACHE_SIZE = CApi.SQLITE_STATUS_PAGECACHE_SIZE;
  public static final int STATUS_MALLOC_COUNT = CApi.SQLITE_STATUS_MALLOC_COUNT;

+  // sqlite3_db_status() ops
  public static final int DBSTATUS_LOOKASIDE_USED = CApi.SQLITE_DBSTATUS_LOOKASIDE_USED;
  public static final int DBSTATUS_CACHE_USED = CApi.SQLITE_DBSTATUS_CACHE_USED;
  public static final int DBSTATUS_SCHEMA_USED = CApi.SQLITE_DBSTATUS_SCHEMA_USED;
@ -168,6 +173,7 @@ public final class Sqlite implements AutoCloseable  {
  public static final int DBSTATUS_CACHE_USED_SHARED = CApi.SQLITE_DBSTATUS_CACHE_USED_SHARED;
  public static final int DBSTATUS_CACHE_SPILL = CApi.SQLITE_DBSTATUS_CACHE_SPILL;

+  // Limits
  public static final int LIMIT_LENGTH = CApi.SQLITE_LIMIT_LENGTH;
  public static final int LIMIT_SQL_LENGTH = CApi.SQLITE_LIMIT_SQL_LENGTH;
  public static final int LIMIT_COLUMN = CApi.SQLITE_LIMIT_COLUMN;
@ -181,15 +187,18 @@ public final class Sqlite implements AutoCloseable  {
  public static final int LIMIT_TRIGGER_DEPTH = CApi.SQLITE_LIMIT_TRIGGER_DEPTH;
  public static final int LIMIT_WORKER_THREADS = CApi.SQLITE_LIMIT_WORKER_THREADS;

+  // sqlite3_prepare_v3() flags
  public static final int PREPARE_PERSISTENT = CApi.SQLITE_PREPARE_PERSISTENT;
  public static final int PREPARE_NO_VTAB = CApi.SQLITE_PREPARE_NO_VTAB;

+  // sqlite3_trace_v2() flags
  public static final int TRACE_STMT = CApi.SQLITE_TRACE_STMT;
  public static final int TRACE_PROFILE = CApi.SQLITE_TRACE_PROFILE;
  public static final int TRACE_ROW = CApi.SQLITE_TRACE_ROW;
  public static final int TRACE_CLOSE = CApi.SQLITE_TRACE_CLOSE;
  public static final int TRACE_ALL = TRACE_STMT | TRACE_PROFILE | TRACE_ROW | TRACE_CLOSE;

+  // sqlite3_db_config() ops
  public static final int DBCONFIG_ENABLE_FKEY = CApi.SQLITE_DBCONFIG_ENABLE_FKEY;
  public static final int DBCONFIG_ENABLE_TRIGGER = CApi.SQLITE_DBCONFIG_ENABLE_TRIGGER;
  public static final int DBCONFIG_ENABLE_FTS3_TOKENIZER = CApi.SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER;
@ -209,10 +218,12 @@ public final class Sqlite implements AutoCloseable  {
  public static final int DBCONFIG_STMT_SCANSTATUS = CApi.SQLITE_DBCONFIG_STMT_SCANSTATUS;
  public static final int DBCONFIG_REVERSE_SCANORDER = CApi.SQLITE_DBCONFIG_REVERSE_SCANORDER;

+  // sqlite3_config() ops
  public static final int CONFIG_SINGLETHREAD = CApi.SQLITE_CONFIG_SINGLETHREAD;
  public static final int CONFIG_MULTITHREAD = CApi.SQLITE_CONFIG_MULTITHREAD;
  public static final int CONFIG_SERIALIZED = CApi.SQLITE_CONFIG_SERIALIZED;

+  // Encodings
  public static final int UTF8 = CApi.SQLITE_UTF8;
  public static final int UTF16 = CApi.SQLITE_UTF16;
  public static final int UTF16LE = CApi.SQLITE_UTF16LE;
@ -220,6 +231,14 @@ public final class Sqlite implements AutoCloseable  {
  /* We elide the UTF16_ALIGNED from this interface because it
     is irrelevant for the Java interface. */

+  // SQL data type IDs
+  public static final int INTEGER = CApi.SQLITE_INTEGER;
+  public static final int FLOAT = CApi.SQLITE_FLOAT;
+  public static final int TEXT = CApi.SQLITE_TEXT;
+  public static final int BLOB = CApi.SQLITE_BLOB;
+  public static final int NULL = CApi.SQLITE_NULL;
+
+  // Authorizer codes.
  public static final int DENY = CApi.SQLITE_DENY;
  public static final int IGNORE = CApi.SQLITE_IGNORE;
  public static final int CREATE_INDEX = CApi.SQLITE_CREATE_INDEX;
--- a/18
+++ b/18
@ -1,5 +1,5 @@
-C JNI\sdoc\supdates.
-D 2023-11-15T06:28:51.535
+C JNI:\sclear\sout\sthe\ssqlite3_context\snative\spointer\safter\scalling\sUDF\scallbacks\swhich\sdo\snot\shave\san\sargv\s(as\swas\salready\sdone\sfor\sthose\swhich\shave\san\sargv).\sAdd\srelated\stests\sand\scode\scommentary.
+D 2023-11-15T08:29:42.027
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@ -239,9 +239,9 @@ F ext/icu/README.txt 7ab7ced8ae78e3a645b57e78570ff589d4c672b71370f5aa9e1cd7024f4
 F ext/icu/icu.c c074519b46baa484bb5396c7e01e051034da8884bad1a1cb7f09bbe6be3f0282
 F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a32075a8
 F ext/jni/GNUmakefile 59eb05f2a363bdfac8d15d66bed624bfe1ff289229184f3861b95f98a19cf4b2
-F ext/jni/README.md 5a556b9fb0a1113f4a5fbf95c0d9c59910bd14ffe048c086528bfb241755a3ff
+F ext/jni/README.md d899789a9082a07b99bf30b1bbb6204ae57c060efcaa634536fa669323918f42
 F ext/jni/jar-dist.make 030aaa4ae71dd86e4ec5e7c1e6cd86f9dfa47c4592c070d2e35157e42498e1fa
-F ext/jni/src/c/sqlite3-jni.c 4fd9906698d296d4e4e4a54c3946461f8506f5b2a13a26cd7b27e0e5c7272bd0
+F ext/jni/src/c/sqlite3-jni.c 6040c0de97644a1fb14bb589ee9f2f4208f6e6b165d14a0e33ed24945b118838
 F ext/jni/src/c/sqlite3-jni.h 913ab8e8fee432ae40f0e387c8231118d17053714703f5ded18202912a8a3fbf
 F ext/jni/src/org/sqlite/jni/annotation/Experimental.java 8603498634e41d0f7c70f661f64e05df64376562ea8f126829fd1e0cdd47e82b
 F ext/jni/src/org/sqlite/jni/annotation/NotNull.java 38e7e58a69b26dc100e458b31dfa3b2a7d67bc36d051325526ef1987d5bc8a24
@ -270,7 +270,7 @@ F ext/jni/src/org/sqlite/jni/capi/SQLFunction.java 0d1e9afc9ff8a2adb94a155b72385
 F ext/jni/src/org/sqlite/jni/capi/SQLTester.java 09bee15aa0eedac68d767ae21d9a6a62a31ade59182a3ccbf036d6463d9e30b1
 F ext/jni/src/org/sqlite/jni/capi/ScalarFunction.java 93b9700fca4c68075ccab12fe0fbbc76c91cafc9f368e835b9bd7cd7732c8615
 F ext/jni/src/org/sqlite/jni/capi/TableColumnMetadata.java addf120e0e76e5be1ff2260daa7ce305ff9b5fafd64153a7a28e9d8f000a815f
-F ext/jni/src/org/sqlite/jni/capi/Tester1.java 11746c7b29cf38f20045f06f6c225be11bcb16bd6a75642987c5c2596f3edd6d
+F ext/jni/src/org/sqlite/jni/capi/Tester1.java e5fa17301b7266c1cbe4bcce67788e08e45871c7c72c153d515abb37e501de0a
 F ext/jni/src/org/sqlite/jni/capi/TraceV2Callback.java 0a25e117a0daae3394a77f24713e36d7b44c67d6e6d30e9e1d56a63442eef723
 F ext/jni/src/org/sqlite/jni/capi/UpdateHookCallback.java c8bdf7848e6599115d601bcc9427ff902cb33129b9be32870ac6808e04b6ae56
 F ext/jni/src/org/sqlite/jni/capi/ValueHolder.java 2ce069f3e007fdbbe1f4e507a5a407fc9679da31a0aa40985e6317ed4d5ec7b5
@ -297,7 +297,7 @@ F ext/jni/src/org/sqlite/jni/test-script-interpreter.md f9f25126127045d051e918fe
 F ext/jni/src/org/sqlite/jni/wrapper1/AggregateFunction.java d5c108b02afd3c63c9e5e53f71f85273c1bfdc461ae526e0a0bb2b25e4df6483
 F ext/jni/src/org/sqlite/jni/wrapper1/ScalarFunction.java 43c43adfb7866098aadaaca1620028a6ec82d5193149970019b1cce9eb59fb03
 F ext/jni/src/org/sqlite/jni/wrapper1/SqlFunction.java 27b141f5914c7cb0e40e90a301d5e05b77f3bd42236834a68031b7086381fafd
-F ext/jni/src/org/sqlite/jni/wrapper1/Sqlite.java 75d4145e7843211f21815e43dfcecf862427558017e586ac3aad02e9bb8419d5
+F ext/jni/src/org/sqlite/jni/wrapper1/Sqlite.java ada39f18e4e3e9d4868dadbc3f7bfe1c6c7fde74fb1fb2954c3f0f70120b805c
 F ext/jni/src/org/sqlite/jni/wrapper1/SqliteException.java 982538ddb4c0719ef87dfa664cd137b09890b546029a7477810bd64d4c47ee35
 F ext/jni/src/org/sqlite/jni/wrapper1/Tester2.java 5e42e6d62aa87409ddbee093b83946c2740b6ac2a39e4868f6a27987677f6a17
 F ext/jni/src/org/sqlite/jni/wrapper1/ValueHolder.java a84e90c43724a69c2ecebd601bc8e5139f869b7d08cb705c77ef757dacdd0593
@ -2140,8 +2140,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93
 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc
 F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e
 F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0
-P 0f4b223102e5dc9142c9d2cb8892b8d3cc476e579420028b93d4e12f4cf94d3e
-R e3ab397266802d21fbd0818f6ef7c03b
+P 1b1f36a206319e99ccaed969893ff95dcf3b8e97ed301544cf3cd3fee2780335
+R 33aee29b5ec7f7f534c74af1bdb413ef
 U stephan
-Z 68da6256258e35616148bab25b4e0c27
+Z 7181ac24798ef67b30f9f54e048aeddb
 # Remove this line to create a well-formed Fossil manifest.
--- a/manifest.uuid
+++ b/manifest.uuid
@ -1 +1 @@
-1b1f36a206319e99ccaed969893ff95dcf3b8e97ed301544cf3cd3fee2780335
+138f40543b26b2e02e27d830d92e30b12cfef5a8dc3f0b58b39c68e1b3c91cc6