From: stephan <stephan@noemail.net>
Date: Tue, 14 Nov 2023 05:33:44 +0000 (+0000)
Subject: JNI: use ByteBuffer.limit() instead of ByteBuffer.capacity() when figuring out where... 
X-Git-Tag: version-3.45.0~164
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4ce5bc2836c9f6cd7af6514c8b1126ac937b1d76;p=thirdparty%2Fsqlite.git

JNI: use ByteBuffer.limit() instead of ByteBuffer.capacity() when figuring out where the logical end of a ByteBuffer is, for reasons explained at length in new code comments. This is unfortunately slower but is the correct way to do it.

FossilOrigin-Name: 51539419edc08ee6c70d8719d0f4d5ad47dd545a7fd9bf01d03a434aabd41d68
---

diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c
index 6668f0b728..79918b3bdb 100644
--- a/ext/jni/src/c/sqlite3-jni.c
+++ b/ext/jni/src/c/sqlite3-jni.c
@@ -661,11 +661,14 @@ struct S3JniGlobalType {
 
       https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#nio_support
 
-      We only store a ref to the following if JNI support for
+      We only store a ref to byteBuffer.klazz if JNI support for
       ByteBuffer is available (which we determine during static init).
     */
-    jclass cByteBuffer       /* global ref to java.nio.ByteBuffer */;
-    jmethodID byteBufferAlloc/* ByteBuffer.allocateDirect() */;
+    struct {
+      jclass klazz       /* global ref to java.nio.ByteBuffer */;
+      jmethodID midAlloc /* ByteBuffer.allocateDirect() */;
+      jmethodID midLimit /* ByteBuffer.limit() */;
+    } byteBuffer;
   } g;
   /*
   ** The list of Java-side auto-extensions
@@ -873,25 +876,52 @@ static jbyte * s3jni__jbyteArray_bytes2(JNIEnv * const env, jbyteArray jBA, jsiz
   if( jBytes ) (*env)->ReleaseByteArrayElements(env, jByteArray, jBytes, JNI_COMMIT)
 
 /*
-** If jbb is-a java.nio.Buffer object and the JNI environment
-** supports it, *pBuf is set to the buffer's memory and *pN is set to
-** its length. If jbb is NULL, not a Buffer, or the JNI environment
-** does not support that operation, *pBuf is set to 0 and *pN is set
-** to 0.
+** If jbb is-a java.nio.Buffer object and the JNI environment supports
+** it, *pBuf is set to the buffer's memory and *pN is set to its
+** limit() (as opposed to its capacity()). If jbb is NULL, not a
+** Buffer, or the JNI environment does not support that operation,
+** *pBuf is set to 0 and *pN is set to 0.
 **
 ** Note that the length of the buffer can be larger than SQLITE_LIMIT
 ** but this function does not know what byte range of the buffer is
 ** required so cannot check for that violation. The caller is required
 ** to ensure that any to-be-bind()ed range fits within SQLITE_LIMIT.
- */
+**
+** Sidebar: it is unfortunate that we cannot get ByteBuffer.limit()
+** via a JNI method like we can for ByteBuffer.capacity(). We instead
+** have to call back into Java to get the limit(). Depending on how
+** the ByteBuffer is used, the limit and capacity might be the same,
+** but when reusing a buffer, the limit may well change whereas the
+** capacity is fixed. The problem with, e.g., read()ing blob data to a
+** ByteBuffer's memory based on its capacity is that Java-level code
+** is restricted to accessing the range specified in
+** ByteBuffer.limit().  If we were to honor only the capacity, we
+** could end up writing to, or reading from, parts of a ByteBuffer
+** which client code itself cannot access without explicitly modifying
+** the limit. The penalty we pay for this correctness is that we must
+** call into Java to get the limit() of every ByteBuffer we work with.
+**
+** An alternative to having to call into ByteBuffer.limit() from here
+** would be to add private native impls of all ByteBuffer-using
+** methods, each of which adds a jint parameter which _must_ be set to
+** theBuffer.limit() by public Java APIs which use those private impls
+** to do the real work.
+*/
 static void s3jni__get_nio_buffer(JNIEnv * const env, jobject jbb, void **pBuf, jint * pN ){
   *pBuf = 0;
   *pN = 0;
   if( jbb ){
     *pBuf = (*env)->GetDirectBufferAddress(env, jbb);
-    *pN = *pBuf ? (jint)(*env)->GetDirectBufferCapacity(env, jbb) : 0
-      /* why the Java limits the buffer length to int but the JNI API
-         uses a jlong for the length is a mystery. */;
+    if( *pBuf ){
+      /*
+      ** Maintenance reminder: do not use
+      ** (*env)->GetDirectBufferCapacity(env,jbb), even though it
+      ** would be much faster, for reasons explained in this
+      ** function's comments.
+      */
+      *pN = (*env)->CallIntMethod(env, jbb, SJG.g.byteBuffer.midLimit);
+      S3JniExceptionIsFatal("Error calling ByteBuffer.limit() method.");
+    }
   }
 }
 #define s3jni_get_nio_buffer(JOBJ,vpOut,jpOut) \
@@ -1097,15 +1127,15 @@ static jstring s3jni_text16_to_jstring(JNIEnv * const env, const void * const p,
 
 /*
 ** Creates a new ByteBuffer instance with a capacity of n. assert()s
-** that SJG.g.cByteBuffer is not 0 and n>0.
+** that SJG.g.byteBuffer.klazz is not 0 and n>0.
 */
 static jobject s3jni__new_ByteBuffer(JNIEnv * const env, int n){
   jobject rv = 0;
-  assert( SJG.g.cByteBuffer );
-  assert( SJG.g.byteBufferAlloc );
+  assert( SJG.g.byteBuffer.klazz );
+  assert( SJG.g.byteBuffer.midAlloc );
   assert( n > 0 );
-  rv = (*env)->CallStaticObjectMethod(env, SJG.g.cByteBuffer,
-                                      SJG.g.byteBufferAlloc, (jint)n);
+  rv = (*env)->CallStaticObjectMethod(env, SJG.g.byteBuffer.klazz,
+                                      SJG.g.byteBuffer.midAlloc, (jint)n);
   S3JniIfThrew {
     S3JniExceptionReport;
     S3JniExceptionClear;
@@ -1124,7 +1154,7 @@ static jobject s3jni__blob_to_ByteBuffer(JNIEnv * const env,
                                          const void * p, int n){
   jobject rv = NULL;
   assert( n >= 0 );
-  if( 0==n || !SJG.g.cByteBuffer ){
+  if( 0==n || !SJG.g.byteBuffer.klazz ){
     return NULL;
   }
   rv = s3jni__new_ByteBuffer(env, n);
@@ -2498,9 +2528,9 @@ static const S3JniNioArgs S3JniNioArgs_empty = {
 ** sqlite3_result_nio_buffer(), and similar methods which take a
 ** ByteBuffer object as either input or output. Populates pArgs and
 ** returns 0 on success, non-0 if the operation should fail. The
-** caller is required to check for SJG.g.cByteBuffer!=0 before calling
+** caller is required to check for SJG.g.byteBuffer.klazz!=0 before calling
 ** this and reporting it in a way appropriate for that routine.  This
-** function may assert() that SJG.g.cByteBuffer is not 0.
+** function may assert() that SJG.g.byteBuffer.klazz is not 0.
 **
 ** The (jBuffer, iOffset, iHowMany) arguments are the (ByteBuffer, offset,
 ** length) arguments to the bind/result method.
@@ -2523,7 +2553,7 @@ static int s3jni_setup_nio_args(
   pArgs->jBuf = jBuffer;
   pArgs->iOffset = iOffset;
   pArgs->iHowMany = iHowMany;
-  assert( SJG.g.cByteBuffer );
+  assert( SJG.g.byteBuffer.klazz );
   if( pArgs->iOffset<0 ){
     return SQLITE_ERROR
       /* SQLITE_MISUSE or SQLITE_RANGE would fit better but we use
@@ -2571,7 +2601,7 @@ S3JniApi(sqlite3_bind_nio_buffer(),jint,1bind_1nio_1buffer)(
   sqlite3_stmt * pStmt = PtrGet_sqlite3_stmt(jpStmt);
   S3JniNioArgs args;
   int rc;
-  if( !pStmt || !SJG.g.cByteBuffer ) return SQLITE_MISUSE;
+  if( !pStmt || !SJG.g.byteBuffer.klazz ) return SQLITE_MISUSE;
   rc = s3jni_setup_nio_args(env, &args, jBuffer, iOffset, iN);
   if(rc){
     return rc;
@@ -2802,7 +2832,7 @@ S3JniApi(sqlite3_blob_read_nio_buffer(),jint,1blob_1read_1nio_1buffer)(
   sqlite3_blob * const b = LongPtrGet_sqlite3_blob(jpBlob);
   S3JniNioArgs args;
   int rc;
-  if( !b || !SJG.g.cByteBuffer || iHowMany<0 ){
+  if( !b || !SJG.g.byteBuffer.klazz || iHowMany<0 ){
     return SQLITE_MISUSE;
   }else if( iTgtOff<0 || iSrcOff<0 ){
     return SQLITE_ERROR
@@ -2847,7 +2877,7 @@ S3JniApi(sqlite3_blob_write_nio_buffer(),jint,1blob_1write_1nio_1buffer)(
   sqlite3_blob * const b = LongPtrGet_sqlite3_blob(jpBlob);
   S3JniNioArgs args;
   int rc;
-  if( !b || !SJG.g.cByteBuffer ){
+  if( !b || !SJG.g.byteBuffer.klazz ){
     return SQLITE_MISUSE;
   }else if( iTgtOff<0 || iSrcOff<0 ){
     return SQLITE_ERROR
@@ -3940,7 +3970,7 @@ S3JniApi(sqlite3_jni_db_error(), jint, 1jni_1db_1error)(
 S3JniApi(sqlite3_jni_supports_nio(), jboolean,1jni_1supports_1nio)(
   JniArgsEnvClass
 ){
-  return SJG.g.cByteBuffer ? JNI_TRUE : JNI_FALSE;
+  return SJG.g.byteBuffer.klazz ? JNI_TRUE : JNI_FALSE;
 }
 
 
@@ -4683,7 +4713,7 @@ S3JniApi(sqlite3_result_nio_buffer(),void,1result_1nio_1buffer)(
   S3JniNioArgs args;
   if( !pCx ){
     return;
-  }else if( !SJG.g.cByteBuffer ){
+  }else if( !SJG.g.byteBuffer.klazz ){
     sqlite3_result_error(
       pCx, "This JVM does not support JNI access to ByteBuffers.", -1
     );
@@ -6257,15 +6287,19 @@ Java_org_sqlite_jni_capi_CApi_init(JniArgsEnvClass){
     unsigned char buf[16] = {0};
     jobject bb = (*env)->NewDirectByteBuffer(env, buf, 16);
     if( bb ){
-      SJG.g.cByteBuffer = S3JniRefGlobal((*env)->GetObjectClass(env, bb));
-      SJG.g.byteBufferAlloc = (*env)->GetStaticMethodID(
-        env, SJG.g.cByteBuffer, "allocateDirect", "(I)Ljava/nio/ByteBuffer;"
+      SJG.g.byteBuffer.klazz = S3JniRefGlobal((*env)->GetObjectClass(env, bb));
+      SJG.g.byteBuffer.midAlloc = (*env)->GetStaticMethodID(
+        env, SJG.g.byteBuffer.klazz, "allocateDirect", "(I)Ljava/nio/ByteBuffer;"
       );
       S3JniExceptionIsFatal("Error getting ByteBuffer.allocateDirect() method.");
+      SJG.g.byteBuffer.midLimit = (*env)->GetMethodID(
+        env, SJG.g.byteBuffer.klazz, "limit", "()I"
+      );
+      S3JniExceptionIsFatal("Error getting ByteBuffer.limit() method.");
       S3JniUnrefLocal(bb);
     }else{
-      SJG.g.cByteBuffer = 0;
-      SJG.g.byteBufferAlloc = 0;
+      SJG.g.byteBuffer.klazz = 0;
+      SJG.g.byteBuffer.midAlloc = 0;
     }
   }
 
diff --git a/ext/jni/src/org/sqlite/jni/capi/CApi.java b/ext/jni/src/org/sqlite/jni/capi/CApi.java
index 090adf94f4..67bdcd676b 100644
--- a/ext/jni/src/org/sqlite/jni/capi/CApi.java
+++ b/ext/jni/src/org/sqlite/jni/capi/CApi.java
@@ -308,12 +308,13 @@ public final class CApi {
      Binds the contents of the given buffer object as a blob.
 
      The byte range of the buffer may be restricted by providing a
-     start index and a number of bytes. beginPos may not be negative
-     but a negative howMany is interpretated as the remainder of the
-     buffer past the given start position.
+     start index and a number of bytes. beginPos may not be negative.
+     Negative howMany is interpretated as the remainder of the buffer
+     past the given start position, up to the buffer's limit() (as
+     opposed its capacity()).
 
-     If beginPos+howMany would extend past the end of the buffer then
-     SQLITE_ERROR is returned.
+     If beginPos+howMany would extend past the limit() of the buffer
+     then SQLITE_ERROR is returned.
 
      If any of the following are true, this function behaves like
      sqlite3_bind_null(): the buffer is null, beginPos is at or past
@@ -347,7 +348,8 @@ public final class CApi {
   );
 
   /**
-     Convenience overload which binds the given buffer's entire contents.
+     Convenience overload which binds the given buffer's entire
+     contents, up to its limit() (as opposed to its capacity()).
   */
   public static int sqlite3_bind_nio_buffer(
     @NotNull sqlite3_stmt stmt, int ndx, @Nullable java.nio.ByteBuffer data
diff --git a/manifest b/manifest
index d62656917f..db02dc3c5c 100644
--- a/manifest
+++ b/manifest
@@ -1,5 +1,5 @@
-C JNI:\sadd\ssqlite3_blob_read_nio_buffer()\sand\siron\sout\sthe\sblob/ByteBuffer\sinterface\ssomewhat.
-D 2023-11-14T04:59:57.233
+C JNI:\suse\sByteBuffer.limit()\sinstead\sof\sByteBuffer.capacity()\swhen\sfiguring\sout\swhere\sthe\slogical\send\sof\sa\sByteBuffer\sis,\sfor\sreasons\sexplained\sat\slength\sin\snew\scode\scomments.\sThis\sis\sunfortunately\sslower\sbut\sis\sthe\scorrect\sway\sto\sdo\sit.
+D 2023-11-14T05:33:44.492
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@@ -241,7 +241,7 @@ F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a3
 F ext/jni/GNUmakefile f2f3a31923293659b95225e932a286af1f2287d75bf88ad6c0fd1b9d9cd020d4
 F ext/jni/README.md ef9ac115e97704ea995d743b4a8334e23c659e5534c3b64065a5405256d5f2f4
 F ext/jni/jar-dist.make 030aaa4ae71dd86e4ec5e7c1e6cd86f9dfa47c4592c070d2e35157e42498e1fa
-F ext/jni/src/c/sqlite3-jni.c 9828d7b6b584c55261e4dd65d86ce4da33daf6cee2966b191ac332ce47efac1c
+F ext/jni/src/c/sqlite3-jni.c 524ca86d59c07db31ad6feb93f2dece977563e1264b903ccfbbdbeff5f288089
 F ext/jni/src/c/sqlite3-jni.h 0ed09051f16f612680603a297fefa2c131c4a7e98e0b41cdd9ece08428b47d48
 F ext/jni/src/org/sqlite/jni/annotation/NotNull.java 02091a8112e33389f1c160f506cd413168c8dfacbeda608a4946c6e3557b7d5a
 F ext/jni/src/org/sqlite/jni/annotation/Nullable.java 0b1879852707f752512d4db9d7edd0d8db2f0c2612316ce1c832715e012ff6ba
@@ -251,7 +251,7 @@ F ext/jni/src/org/sqlite/jni/capi/AggregateFunction.java 0b72cdff61533b564d65b63
 F ext/jni/src/org/sqlite/jni/capi/AuthorizerCallback.java c045a5b47e02bb5f1af91973814a905f12048c428a3504fbc5266d1c1be3de5a
 F ext/jni/src/org/sqlite/jni/capi/AutoExtensionCallback.java 74cc4998a73d6563542ecb90804a3c4f4e828cb4bd69e61226d1a51f4646e759
 F ext/jni/src/org/sqlite/jni/capi/BusyHandlerCallback.java 7b8e19810c42b0ad21a04b5d8c804b32ee5905d137148703f16a75b612c380ca
-F ext/jni/src/org/sqlite/jni/capi/CApi.java a7c53a3226c6826ada00752a651a31e6cce3b8d741a02b2d45cb0d1e3dfc3a80
+F ext/jni/src/org/sqlite/jni/capi/CApi.java 7b2eae29f21db915bd358f3fab1eb9f1a4cbc8b2cc4c78aab7309cd69b71958a
 F ext/jni/src/org/sqlite/jni/capi/CallbackProxy.java 57e2d275dcebe690b1fc1f3d34eb96879b2d7039bce30b563aee547bf45d8a8b
 F ext/jni/src/org/sqlite/jni/capi/CollationCallback.java e29bcfc540fdd343e2f5cca4d27235113f2886acb13380686756d5cabdfd065a
 F ext/jni/src/org/sqlite/jni/capi/CollationNeededCallback.java 5bfa226a8e7a92e804fd52d6e42b4c7b875fa7a94f8e2c330af8cc244a8920ab
@@ -2139,8 +2139,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 46656b354311ec0a36832af1c4ccb3b6a244aa55cfb3681e25c3f42b13b387dd
-R 4e0b6f66f2c79b3b0f3d9c780b63f86e
+P 7df317b448a09ae77e2c68cc901fdb6d56a2246c1313f06bebd1f3e53f02c19b
+R dddfec0befd08ab210a022ab61f478a4
 U stephan
-Z 5994994aa13f973034ef1e21e15b70de
+Z 49fb00c3c45f37f7c83250698ea520ef
 # Remove this line to create a well-formed Fossil manifest.
diff --git a/manifest.uuid b/manifest.uuid
index 8b049893ca..fde4bb75f6 100644
--- a/manifest.uuid
+++ b/manifest.uuid
@@ -1 +1 @@
-7df317b448a09ae77e2c68cc901fdb6d56a2246c1313f06bebd1f3e53f02c19b
\ No newline at end of file
+51539419edc08ee6c70d8719d0f4d5ad47dd545a7fd9bf01d03a434aabd41d68
\ No newline at end of file