bug#63590: 29.0.90; can't load sqlite extension

[Lennart Vogelsang via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Ahh, I just wanted to answer you, I just noticed that about the tests 
too. Thank you! Your patch works for me,
just one small thing: sqlite extension loading can also fail because of 
other reasons (e.g. if the shared library
does not exist). Currently your patch would leave sqlite extension 
loading enabled in that case, I think?

I would also argue that it would make sense to actually report the error 
of the extension loading (when  the dynamic library file does not exist, 
or the extension is invalid). Maybe something like this:

diff --git a/src/sqlite.c b/src/sqlite.c
index 0361514766a..4be8acc9a94 100644
--- a/src/sqlite.c
+++ b/src/sqlite.c
@@ -23,6 +23,8 @@ Copyright (C) 2021-2023 Free Software Foundation, Inc.
     https://github.com/syohex/emacs-sqlite3  */

  #include <config.h>
+
+#include <c-strcase.h>
  #include "lisp.h"
  #include "coding.h"

@@ -686,7 +688,8 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
    /* Add names of useful and free modules here.  */
    const char *allowlist[3] = { "pcre", "csvtable", NULL };
    char *name = SSDATA (Ffile_name_nondirectory (module));
-  /* Possibly skip past a common prefix.  */
+  /* Possibly skip past a common prefix (libsqlite3_mod_ is used by
+     Debian, see https://packages.debian.org/source/sid/sqliteodbc).  */
    const char *prefix = "libsqlite3_mod_";
    if (!strncmp (name, prefix, strlen (prefix)))
      name += strlen (prefix);
@@ -697,7 +700,7 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
        if (strlen (*allow) < strlen (name)
        && !strncmp (*allow, name, strlen (*allow))
        && (!strcmp (name + strlen (*allow), ".so")
-          || !strcmp (name + strlen (*allow), ".DLL")))
+          || !strcasecmp (name + strlen (*allow), ".dll")))
      {
        do_allow = true;
        break;
@@ -707,12 +710,32 @@ DEFUN ("sqlite-load-extension", 
Fsqlite_load_extension,
    if (!do_allow)
      xsignal1 (Qsqlite_error, build_string ("Module name not on 
allowlist"));

-  int result = sqlite3_load_extension
-               (XSQLITE (db)->db,
-            SSDATA (ENCODE_FILE (Fexpand_file_name (module, Qnil))),
-            NULL, NULL);
-  if (result ==  SQLITE_OK)
-    return Qt;
+  /* Expand all Lisp data explicitly, so as to avoid signaling an
+     error while extension loading is enabled -- we don't want to
+     "leak" this outside this function.  */
+  sqlite3 *sdb = XSQLITE (db)->db;
+  char *ext_fn = SSDATA (ENCODE_FILE (Fexpand_file_name (module, Qnil)));
+  /* Temporarily enable loading extensions via the C API.  */
+  int result = sqlite3_db_config (sdb, 
SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION, 1,
+                  NULL);
+  if (result == SQLITE_OK)
+    {
+      /* save error from sqlite */
+      char *errmsg;
+      result = sqlite3_load_extension (sdb, ext_fn, NULL, &errmsg);
+      /* Disable loading extensions via C API.  */
+      sqlite3_db_config (sdb, SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION,
+             0, NULL);
+      if (result == SQLITE_OK)
+    {
+      return Qt;
+    }
+      else
+    {
+      xsignal1 (Qsqlite_error, build_string (errmsg));
+      sqlite_free (errmsg);
+    }
+    }
    return Qnil;
  }
  #endif /* HAVE_SQLITE3_LOAD_EXTENSION */

That way, the test also correctly fails as we signal the error from the 
extension loading.

Regarding csv.c, yes I forgot to mention that. I admit for testing 
purposes I changed the name there (to sqlite3_extension_init, which 
sqlite also always accepts). Thank you for pointing me to the real 
extension. Just out of curiosity, as there are a handful of useful 
sqlite extensions out there, could there be a way to make the allow list 
a bit more lenient? Maybe as a build configure feature allowing us to 
specify other extensions that are allowed to be loaded.

On 5/20/23 11:59 AM, Eli Zaretskii wrote:
>> Date: Fri, 19 May 2023 15:25:21 +0200
>> From:  Lennart Vogelsang via "Bug reports for GNU Emacs,
>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> To reproduce, I've created an empty folder, cd'ed into it, started
>> emacs -Q, copied the sqlite's csv extension source code [0] into
>> csvtable.c,
>> compiled it with
>>
>>        gcc -O3 -Wall -Wno-unknown-pragmas -fPIC -shared -lm -o
>> csvtable.so csvtable.c
>>
>> and executed the following elisp forms in the scratch buffer:
>>
>>        (setq-local mydb (sqlite-open))
>>        (sqlite-load-extension mydb "./csvtable.so")
>>
>> I get a nil return value from the second expression, indicating
>> that it did not load the extension (verified by using the `csv` module
>> in a `sqlite-execute` call). If I try the same from the `sqlite3` cli
>> interface, it works:
>>
>>        .load ./csvtable.so
> I think you made one more change to csv.c: you renamed the function
> sqlite3_csv_init to the name sqlite3_csvtable_init.  Otherwise, the
> loading would fail, because sqlite3's cli will not find the entry
> function it expects.
>
> More importantly: the csv.c source file to which you point, viz.:
>
>    https://www.sqlite.org/src/artifact?ci=trunk&filename=ext/misc/csv.c
>
> is NOT the source file of the libsqlite3_mod_csvtable.so extension
> distributed by Debian, which we currently have on the "allow list", it
> is a different extension.  The source of csvtable is here:
>
>    https://packages.debian.org/sid/libsqlite3-mod-csvtable