Open Bug 486312 Opened 11 years ago Updated Last year

Load Sqlite Extension

Categories

(Toolkit :: Storage, enhancement, P5)

enhancement

Tracking

()

REOPENED

People

(Reporter: rldhont, Unassigned)

Details

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: 

I would like to load Sqlite Extension to a Storage Connection.

Reproducible: Always

Actual Results:  
No API to load Extension

Expected Results:  
Add a method to mozIStroageConnection to load a file (nsIFile) wich is a Sqlite Extension.
This would need tests before we would ever consider taking this patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
Priority: -- → P4
er, did you upload the right patch?  It looks like this adds a new copy of sqlite somewhere?
Yeah, this patch changes mozIStorageConnection.idl and mozStorageConnection.cpp, and it adds files for creating an Sqlite Extension to use in the unit test.
Attachment #370816 - Flags: review?(sdwilsh)
It'll probably be about a week before I can take a look at this.  Need to prioritize work that will make it for Gecko 1.9.1/Firefox 3.5.
Attachment #370816 - Flags: review?(sdwilsh) → review-
Comment on attachment 370816 [details] [diff] [review]
New Patch with unit test

In general, you need to make sure you are following http://mxr.mozilla.org/mozilla-central/source/storage/style.txt.  Also, you should be aware of bug 488379 which will likely land before your patch.  You should probably be making your changes on top of the patch in that bug.

>+  /**
>+   * load an Sqlite Extension.
>+   *
>+   * @param aFile the Sqlite extension lib.
>+   * @param aEntryPoint the Sqlite extension lib entry point, NULL if it's sqlite3_extension_init.
>+   *
>+   */
>+  void loadExtension(in nsIFile aFile, in string aEntryPoint);
nit: please format your java-doc style comment like executAsync's java-doc style comment in this file.

aEntryPoint is really really vague.  Seems to me like this is the function used to initialize the extension right?  Let's call it something else, and have a better description.  aFile and the method description need better descriptions too.

>+++ b/storage/src/mozStorageConnection.cpp	Fri Apr 03 10:48:30 2009 +0200
>+  int srv;
>+  nsresult rv;
Please declare variables at first use, not C-style

>+  rv = aFile->GetPath(filePath);
You should check rv

>+  if (aEntryPoint == NULL)
NULL would be 0, so you can just pass it to sqlite3_load_extension, no?

>+  if (srv != SQLITE_OK)
>+    NS_WARNING("sqlite3_load_extension failed!");
You should really throw if we cannot load the extension.

>+  srv = sqlite3_enable_load_extension(mDBConn, 0);
You don't check this (either call) for SQLITE_OK

>+++ b/storage/test/Makefile.in	Fri Apr 03 10:48:30 2009 +0200
>+DIRS = ext
Please use the same format as REQUIRES, even if you have only one directory to add.  Also, please use a better name than "ext"

>diff -r 0f030665e3ba storage/test/ext/Makefile.in
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/storage/test/ext/Makefile.in	Fri Apr 03 10:48:30 2009 +0200
>@@ -0,0 +1,40 @@
>+# @author D'Hont René-Luc - 3Liz
>+# http://www.3liz.com
>+#
We need a license block here

>+#
>+# We are in storage/test/ext
>+#
Comment isn't useful

>+
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
no tabs please for spacing here.  See the style guidelines

>diff -r 0f030665e3ba storage/test/ext/sqlite3.h
Please don't include sqltie3.h in the repo again.  We already have it in db/sqlite/src

>diff -r 0f030665e3ba storage/test/ext/sqlite3ext.h
If we need this, it should be in db/sqlite3/src.  You'll also have to update http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/README.MOZILLA accordingly.

>+++ b/storage/test/ext/test_loadext.c	Fri Apr 03 10:48:30 2009 +0200
while it's nice that you are just including SQLite's test file here, I'd prefer if we write our own simple extension here.  They already run this test on their code.

>+++ b/storage/test/unit/test_storage_load_extension.js	Fri Apr 03 10:48:30 2009 +0200
Need a license block

>+function get_test_ext() 
>+{
>+  var lib = dirSvc.get("CurProcD", Ci.nsIFile);
>+  lib = lib.parent;
>+  lib.append('lib');
>+
>+  // Linux
>+  var ext = lib.clone();
>+  ext.append('libtestloadext.so');
>+  if (ext.exists())
>+    return ext;
>+  
>+  // Windows
>+  ext = lib.clone();
>+  ext.append('testloadext.dll');
>+  if (ext.exists())
>+    return ext;
What about OS X?

>+function test_half() 
>+{
>+  var ext = get_test_ext();
>+  if (!ext)
>+    do_trow('Extension library not found');
>+  
>+  var db = getOpenedDatabase();
>+
>+  // No extension
>+  try {
>+    var stmt = createStatement("SELECT half(1.0);");
>+  } catch(e) {
>+    do_check_eq(db.lastErrorString, 'no such function: half');
>+  }
nit: style is:
try {
  // code
}
catch(e) {
}

>+  do_check_true(stmt.executeStep());
executeStep needs to be wrapped in a try block

>+  stmt.reset();
>+  stmt.finalize();
do not need to reset if you are going to finalize - and but finalize in a finally block

>+function run_test()
>+{
>+  test_half();
Use an array of tests, like other files do please so it's easy to add more in the future.
Now, just because I'm doing a review, doesn't mean we are going to take this feature yet.  I'm not sure of the justification for this patch yet.  As of now, the test files are only adding a function, be we can already do that with mozIStorageConnection::createFunction and mozIStorageConnection::createAggregateFunction.  Additionally, this needs to go through a security review before it can land.

I'm also concerned about how this would affect our ability to upgrade SQLite in security updates.  If changing the SQLite version means that we break binary extensions for SQLite, supporting this feature essentially locks us into a version of SQLite once we ship.  That'd be really unfortunate.
I proposed this feature because I would like to use Spatialite, http://www.gaia-gis.it/spatialite/. This extension transform an SQLite database in a Geographic database. It adds functions and types to manipulate geometry and network. But I knew that there is some security review needed.
(In reply to comment #8)
> I'm also concerned about how this would affect our ability to upgrade SQLite in
> security updates.  If changing the SQLite version means that we break binary
> extensions for SQLite, supporting this feature essentially locks us into a
> version of SQLite once we ship.  That'd be really unfortunate.

Yeah, we really need the ability to upgrade sqlite on the branch without breaking extensions. If there were a serious security issue in a version of sqlite, I'd rather not break lots of extensions in the midst of a firedrill (and then cause those users not to upgrade to the shipped version because it broke their extensions!).
René-Luc, is the intent to use this in a Firefox extension, or instead something using the platform; for example, a XULRunner app?
It's firstly for my own XulRunner App, but I thought, a DB geographically aware will be usefull in Messaging and why not in a Browser or in a Mobile Browser...
It seems like security firedrills related to SQLite are unlikely to have anything to do with the SQLite core.  Unless the parameter binding code (which is relatively simple and unchanging) goes rogue,  the security issues are likely to be the fault of the code using mozStorage failing to properly escape/bind parameters or otherwise be convinced by malicious input to do bad things.

I do think concerns about code maintenance/complexity and Firefox extension concerns are appropriate.  It might make sense to have such a feature for the use of XULRunner but explicitly disable it for Firefox.  Yet that complicates testing mozStorage.  Likewise, having to build binaries for multiple platforms for testing purposes is likely to add some burden when changes in the mozilla build system or significant changes to SQLite occur.

My concern would be that this feature gets added, never sees any real world usage, but requires maintenance and effort forever.  My suggestion would be that the functionality demonstrates its worth first.  The 'try' server (https://wiki.mozilla.org/Build:TryServer) makes it easy to produce builds for all platforms with patches applied.  I would assert this makes it feasible for the development of something that uses Spatialite to happen without first landing the code in mozilla-central.  Arguably, it also would show longevity of interest in helping maintain the code against trunk.

(Note: the only potential flaw is that use of the 'try' server does require an hg account... but presumably something could be arranged.)
It is currently impossible to open any sqlite databases that have constraints relying on external functions. Not being able to import these external functions is therefore a severe handicap. This bites me when using SQLiteManager from Firefox which is probably the primary use case.

See http://code.google.com/p/sqlite-manager/issues/detail?id=217 for the related bug in SQLiteManager

A simpler solution than allowing an API to load extensions would be to simply add a function to turn on support for loading extensions (i.e. the sqlite3_enable_load_extension call) - this is much simpler, and the actual loading of extensions can then be done from SQL commands.
Hello, any hope to have the solution proposed in comment #14 implemented? I use SQLite Manager and would like to be able to use "extension-functions.c" which is available at http://www.sqlite.org/contrib and provides mathematical and string extension functions for SQLite queries. 

The extension works ok with SQLite CLI recompiled with the --enable-dynamic-extensions option (which defaults to yes), but fails under SQLite Manager as loading extensions via SQL statements is currently disabled in Firefox. Thanks.
Priority: P4 → P5
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
You need to log in before you can comment on or make changes to this bug.