From: stephan Date: Wed, 23 Aug 2023 00:17:28 +0000 (+0000) Subject: Improve C-side exception handling from Java-side UDF callbacks. X-Git-Tag: version-3.44.0~305^2~8 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=336bc8a28114364d42987dc1c21f48a6910080f9;p=thirdparty%2Fsqlite.git Improve C-side exception handling from Java-side UDF callbacks. FossilOrigin-Name: aebbc24afb339ed07b7cd767fbc0d25f3e9c3d9bb5ef3d1c10b04b605c7261bc --- diff --git a/ext/jni/README.md b/ext/jni/README.md index 395365292f..5e100c4f07 100644 --- a/ext/jni/README.md +++ b/ext/jni/README.md @@ -105,7 +105,9 @@ Client-defined callbacks _must never throw exceptions_ unless _very explicitly documented_ as being throw-safe. Exceptions are generally reserved for higher-level bindings which are constructed to specifically deal with them and ensure that they do not leak C-level -resources. +resources. In some cases, callback handlers (see below) are permitted +to throw, in which cases they get translated to C-level result codes +and/or messages. Unwieldy Constructs are Re-mapped diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c index f1bc049545..3159116f72 100644 --- a/ext/jni/src/c/sqlite3-jni.c +++ b/ext/jni/src/c/sqlite3-jni.c @@ -1577,21 +1577,46 @@ error_oom: return SQLITE_NOMEM; } -static int udf_report_exception(sqlite3_context * cx, - const char *zFuncName, - const char *zFuncType){ - int rc; - char * z = - sqlite3_mprintf("Client-defined function %s.%s() threw. It should " - "not do that.", - zFuncName ? zFuncName : "", zFuncType); - if(z){ - sqlite3_result_error(cx, z, -1); - sqlite3_free(z); - rc = SQLITE_ERROR; +/* +** Must be called immediately after a Java-side UDF callback throws. +** If translateToErr is true then it sets the exception's message in +** the result error using sqlite3_result_error(). If translateToErr is +** false then it emits a warning that the function threw but should +** not do so. In either case, it clears the exception state. +** +** Returns SQLITE_NOMEM if an allocation fails, else SQLITE_ERROR. In +** the latter case it calls sqlite3_result_error_nomem(). +*/ +static int udf_report_exception(JNIEnv * const env, int translateToErr, + sqlite3_context * cx, + const char *zFuncName, const char *zFuncType ){ + jthrowable const ex = (*env)->ExceptionOccurred(env); + int rc = SQLITE_ERROR; + + assert(ex && "This must only be called when a Java exception is pending."); + if( translateToErr ){ + char * zMsg; + char * z; + + EXCEPTION_CLEAR; + zMsg = s3jni_exception_error_msg(env, ex); + z = sqlite3_mprintf("Client-defined SQL function %s.%s() threw: %s", + zFuncName ? zFuncName : "", zFuncType, + zMsg ? zMsg : "Unknown exception" ); + sqlite3_free(zMsg); + if( z ){ + sqlite3_result_error(cx, z, -1); + sqlite3_free(z); + }else{ + sqlite3_result_error_nomem(cx); + rc = SQLITE_NOMEM; + } }else{ - sqlite3_result_error_nomem(cx); - rc = SQLITE_NOMEM; + MARKER(("Client-defined SQL function %s.%s() threw. " + "It should not do that.\n", + zFuncName ? zFuncName : "", zFuncType)); + (*env)->ExceptionDescribe( env ); + EXCEPTION_CLEAR; } return rc; } @@ -1600,24 +1625,25 @@ static int udf_report_exception(sqlite3_context * cx, Sets up the state for calling a Java-side xFunc/xStep/xInverse() UDF, calls it, and returns 0 on success. */ -static int udf_xFSI(sqlite3_context* pCx, int argc, - sqlite3_value** argv, - S3JniUdf * s, +static int udf_xFSI(sqlite3_context* const pCx, int argc, + sqlite3_value** const argv, + S3JniUdf * const s, jmethodID xMethodID, - const char * zFuncType){ + const char * const zFuncType){ LocalJniGetEnv; udf_jargs args = {0,0}; int rc = udf_args(env, pCx, argc, argv, &args.jcx, &args.jargv); - //MARKER(("%s.%s() pCx = %p\n", s->zFuncName, zFuncType, pCx)); - if(rc) return rc; + //MARKER(("UDF::%s.%s()\n", s->zFuncName, zFuncType)); + if(rc) return rc; if( UDF_SCALAR != s->type ){ rc = udf_setAggregateContext(env, args.jcx, pCx, 0); } if( 0 == rc ){ (*env)->CallVoidMethod(env, s->jObj, xMethodID, args.jcx, args.jargv); IFTHREW{ - rc = udf_report_exception(pCx, s->zFuncName, zFuncType); + rc = udf_report_exception(env, 'F'==zFuncType[1]/*xFunc*/, pCx, + s->zFuncName, zFuncType); } } UNREF_L(args.jcx); @@ -1635,19 +1661,21 @@ static int udf_xFV(sqlite3_context* cx, S3JniUdf * s, LocalJniGetEnv; jobject jcx = new_sqlite3_context_wrapper(env, cx); int rc = 0; + int const isFinal = 'F'==zFuncType[1]/*xFinal*/; //MARKER(("%s.%s() cx = %p\n", s->zFuncName, zFuncType, cx)); if(!jcx){ - sqlite3_result_error_nomem(cx); + if( isFinal ) sqlite3_result_error_nomem(cx); return SQLITE_NOMEM; } //MARKER(("UDF::%s.%s()\n", s->zFuncName, zFuncType)); if( UDF_SCALAR != s->type ){ - rc = udf_setAggregateContext(env, jcx, cx, 1); + rc = udf_setAggregateContext(env, jcx, cx, isFinal); } if( 0 == rc ){ (*env)->CallVoidMethod(env, s->jObj, xMethodID, jcx); IFTHREW{ - rc = udf_report_exception(cx,s->zFuncName, zFuncType); + rc = udf_report_exception(env, isFinal, cx, s->zFuncName, + zFuncType); } } UNREF_L(jcx); @@ -3599,8 +3627,7 @@ static void s3jni_fts5_extension_function(Fts5ExtensionApi const *pApi, (*env)->CallVoidMethod(env, pAux->jObj, pAux->jmid, jFXA, jpFts, jpCx, jArgv); IFTHREW{ - EXCEPTION_CLEAR; - udf_report_exception(pCx, pAux->zFuncName, "xFunction"); + udf_report_exception(env, 1, pCx, pAux->zFuncName, "xFunction"); } UNREF_L(jpFts); UNREF_L(jpCx); diff --git a/ext/jni/src/org/sqlite/jni/SQLFunction.java b/ext/jni/src/org/sqlite/jni/SQLFunction.java index 28775608ad..c8e87ff827 100644 --- a/ext/jni/src/org/sqlite/jni/SQLFunction.java +++ b/ext/jni/src/org/sqlite/jni/SQLFunction.java @@ -98,7 +98,11 @@ public abstract class SQLFunction { //! Subclass for creating scalar functions. public static abstract class Scalar extends SQLFunction { - //! As for the xFunc() argument of the C API's sqlite3_create_function() + /** + As for the xFunc() argument of the C API's + sqlite3_create_function(). If this function throws, it is + translated into an sqlite3_result_error(). + */ public abstract void xFunc(sqlite3_context cx, sqlite3_value[] args); /** @@ -116,10 +120,18 @@ public abstract class SQLFunction { */ public static abstract class Aggregate extends SQLFunction { - //! As for the xStep() argument of the C API's sqlite3_create_function() + /** + As for the xStep() argument of the C API's + sqlite3_create_function(). If this function throws, the + exception is not propagated and a warning might be emitted to a + debugging channel. + */ public abstract void xStep(sqlite3_context cx, sqlite3_value[] args); - //! As for the xFinal() argument of the C API's sqlite3_create_function() + /** + As for the xFinal() argument of the C API's sqlite3_create_function(). + If this function throws, it is translated into an sqlite3_result_error(). + */ public abstract void xFinal(sqlite3_context cx); /** @@ -165,10 +177,18 @@ public abstract class SQLFunction { */ public static abstract class Window extends Aggregate { - //! As for the xInverse() argument of the C API's sqlite3_create_window_function() + /** + As for the xInverse() argument of the C API's + sqlite3_create_window_function(). If this function throws, the + exception is not propagated and a warning might be emitted + to a debugging channel. + */ public abstract void xInverse(sqlite3_context cx, sqlite3_value[] args); - //! As for the xValue() argument of the C API's sqlite3_create_window_function() + /** + As for the xValue() argument of the C API's sqlite3_create_window_function(). + See xInverse() for the fate of any exceptions this throws. + */ public abstract void xValue(sqlite3_context cx); } } diff --git a/ext/jni/src/org/sqlite/jni/Tester1.java b/ext/jni/src/org/sqlite/jni/Tester1.java index cc221d1013..c075a2d036 100644 --- a/ext/jni/src/org/sqlite/jni/Tester1.java +++ b/ext/jni/src/org/sqlite/jni/Tester1.java @@ -158,16 +158,22 @@ public class Tester1 implements Runnable { execSql(db, true, sql); } - static sqlite3_stmt prepare(sqlite3 db, String sql){ + static sqlite3_stmt prepare(sqlite3 db, boolean throwOnError, String sql){ final OutputPointer.sqlite3_stmt outStmt = new OutputPointer.sqlite3_stmt(); int rc = sqlite3_prepare(db, sql, outStmt); - affirm( 0 == rc ); + if( throwOnError ){ + affirm( 0 == rc ); + } final sqlite3_stmt rv = outStmt.take(); affirm( null == outStmt.get() ); - affirm( 0 != rv.getNativePointer() ); + if( throwOnError ){ + affirm( 0 != rv.getNativePointer() ); + } return rv; } - + static sqlite3_stmt prepare(sqlite3 db, String sql){ + return prepare(db, true, sql); + } private void showCompileOption(){ int i = 0; String optName; @@ -598,6 +604,47 @@ public class Tester1 implements Runnable { affirm( xDestroyCalled.value ); } + private void testUdfThrows(){ + final sqlite3 db = createNewDb(); + final ValueHolder xFuncAccum = new ValueHolder<>(0); + + SQLFunction funcAgg = new SQLFunction.Aggregate(){ + @Override public void xStep(sqlite3_context cx, sqlite3_value[] args){ + /** Throwing from here should emit loud noise on stdout or stderr + but the exception is supressed because we have no way to inform + sqlite about it from these callbacks. */ + //throw new RuntimeException("Throwing from an xStep"); + } + @Override public void xFinal(sqlite3_context cx){ + throw new RuntimeException("Throwing from an xFinal"); + } + }; + int rc = sqlite3_create_function(db, "myagg", 1, SQLITE_UTF8, funcAgg); + affirm(0 == rc); + affirm(0 == xFuncAccum.value); + sqlite3_stmt stmt = prepare(db, "SELECT myagg(1)"); + rc = sqlite3_step(stmt); + sqlite3_finalize(stmt); + affirm( 0 != rc ); + affirm( sqlite3_errmsg(db).indexOf("an xFinal") > 0 ); + + SQLFunction funcSc = new SQLFunction.Scalar(){ + @Override public void xFunc(sqlite3_context cx, sqlite3_value[] args){ + throw new RuntimeException("Throwing from an xFunc"); + } + }; + rc = sqlite3_create_function(db, "mysca", 0, SQLITE_UTF8, funcSc); + affirm(0 == rc); + affirm(0 == xFuncAccum.value); + stmt = prepare(db, "SELECT mysca()"); + rc = sqlite3_step(stmt); + sqlite3_finalize(stmt); + affirm( 0 != rc ); + affirm( sqlite3_errmsg(db).indexOf("an xFunc") > 0 ); + + sqlite3_close_v2(db); + } + private void testUdfJavaObject(){ final sqlite3 db = createNewDb(); final ValueHolder testResult = new ValueHolder<>(db); diff --git a/manifest b/manifest index 29963f490c..56dd2871d2 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Minor\sTester1.java\scleanups. -D 2023-08-22T23:00:44.674 +C Improve\sC-side\sexception\shandling\sfrom\sJava-side\sUDF\scallbacks. +D 2023-08-23T00:17:28.362 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -233,9 +233,9 @@ F ext/icu/README.txt 7ab7ced8ae78e3a645b57e78570ff589d4c672b71370f5aa9e1cd7024f4 F ext/icu/icu.c c074519b46baa484bb5396c7e01e051034da8884bad1a1cb7f09bbe6be3f0282 F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a32075a8 F ext/jni/GNUmakefile a38d7c5ad4ab1e209e2a9352ff06add1d9a0bc666351714bfc8858625330c16b -F ext/jni/README.md 975b35173debbbf3a4ab7166e14d2ffa2bacff9b6850414f09cc919805e81ba4 +F ext/jni/README.md ddcc6be0c0d65f1e2fd687de9f40d38c82630fd61f83cc9550773caa19dd8be1 F ext/jni/jar-dist.make 9a03d10dbb5a74c724bfec4b76fd9e4c9865cbbc858d731cb48f38ac897d73a3 -F ext/jni/src/c/sqlite3-jni.c 1064441d33cef541c9c9c84fb5093573a0767bb36563e6ea538c355b6148c4c6 +F ext/jni/src/c/sqlite3-jni.c 8e7dac68246edec8d85e34104d5c749c544ae10c55b1bbf6eeff116447da7f54 F ext/jni/src/c/sqlite3-jni.h 8b0ab1a3f0f92b75d4ff50db4a88b66a137cfb561268eb15bb3993ed174dbb74 F ext/jni/src/org/sqlite/jni/Authorizer.java 1308988f7f40579ea0e4deeaec3c6be971630566bd021c31367fe3f5140db892 F ext/jni/src/org/sqlite/jni/AutoExtension.java 3b62c915e45ce73f63343ca9195ec63592244d616a1908b7587bdd45de1b97dd @@ -254,9 +254,9 @@ F ext/jni/src/org/sqlite/jni/OutputPointer.java 464ea85c3eba673a7b575545f69fcd8a F ext/jni/src/org/sqlite/jni/ProgressHandler.java 6f62053a828a572de809828b1ee495380677e87daa29a1c57a0e2c06b0a131dc F ext/jni/src/org/sqlite/jni/ResultCode.java ba701f20213a5f259e94cfbfdd36eb7ac7ce7797f2c6c7fca2004ff12ce20f86 F ext/jni/src/org/sqlite/jni/RollbackHook.java b04c8abcc6ade44a8a57129e33765793f69df0ba909e49ba18d73f4268d92564 -F ext/jni/src/org/sqlite/jni/SQLFunction.java 8c1ad92c35bcc1b2f7256cf6e229b31340ed6d1a404d487f0a9adb28ba7fc332 +F ext/jni/src/org/sqlite/jni/SQLFunction.java f697cf2a81c4119f2baf0682af689686f0466f1dd83dba00885f5603e693fe16 F ext/jni/src/org/sqlite/jni/SQLite3Jni.java 2f36370cfdec01d309720392b2c3e4af6afce0b6ece8188b5c3ed688a5a1e63a -F ext/jni/src/org/sqlite/jni/Tester1.java 1a9d9a0ce93aa105dc409273bb50bf10f15e8b546d9ef0922b14e69dc3f14107 +F ext/jni/src/org/sqlite/jni/Tester1.java 93bd76f2294fa2f592395c9d823adb38a9be3846bff00debeff02310f4e9e6be F ext/jni/src/org/sqlite/jni/TesterFts5.java de095e3b701fba0c56d7b8b2993dc22bcbaa9de8f992904a93729ad729a91576 F ext/jni/src/org/sqlite/jni/Tracer.java a5cece9f947b0af27669b8baec300b6dd7ff859c3e6a6e4a1bd8b50f9714775d F ext/jni/src/org/sqlite/jni/UpdateHook.java e58645a1727f8a9bbe72dc072ec5b40d9f9362cb0aa24acfe93f49ff56a9016d @@ -2092,8 +2092,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 56b2a077ace6e6ad5834e1a597b710f212a5b7d5db5b9a27a41f2aa0f6952c55 -R 786162271678eb8ff4639708ab69b4fa +P 70d936953ba55cfed32efe7e3a9d4b71da9a7ffc8818b6540471e0bf311bc688 +R f4b3990a7d714ecf1a549380689ce9aa U stephan -Z 0962b73394fcdbf4989509de32c7e31a +Z b8494717453324d170d4924e0442cf8e # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 767f632dca..d1c49b53b7 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -70d936953ba55cfed32efe7e3a9d4b71da9a7ffc8818b6540471e0bf311bc688 \ No newline at end of file +aebbc24afb339ed07b7cd767fbc0d25f3e9c3d9bb5ef3d1c10b04b605c7261bc \ No newline at end of file