[asterisk-dev] [Code Review]: Add SQLite 3 realtime driver

[Terry Wilson]

> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, line 115
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line115>
> >
> >     You need to add 1 to this, because the internal calculations of ast_str_thread_get() do not accomodate for the terminating NULL.

Nope. sizeof takes care of that. sizeof("hi") == 3.


> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, lines 136-139
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line136>
> >
> >     We don't have any columns in Asterisk realtime that have a single quote character embedded in the name, so this is also superfluous.

I disagree. Just because we don't have them now, doesn't mean someone won't use their own crazy scheme sometime. And columns most certainly are something that people could define themselves and pass in if we ever opened up realtime to AMI, etc. With that said, I'm not a huge fan of the SQL escaping abilities of SQLite functions since they don't tend to protect against multiple special characters. SELECT * FROM sipregs WHERE "lastms" IS NOT NULL;DROP TABLE "sipregs", etc. I haven't tested how well the escaping functions really work

SELECT * FROM "%s" WHERE "%s" %s) with sipregs, lastms" IS NOT NULL;DROP TABLE "sipregs, "" would be particularly not pleasant.

We can't assume that column names aren't going to be user-generated data at some point. Having realtime AMI stuff could be very handy when combined with a built-in SQLite 3 db. You could do a user interface without a back-end scripting language, but without all of the complex config handling the way we currently do it for Asterisk GUI.

I may just have to break down and write or find a suitably licensed SQL escaping function. I hate sql escaping. I would much rather use parameter binding, but since it doesn't handle column names it just isn't enough in this case. Generally dynamic column names are a horrible idea, but realtime doesn't really have a definition of column names. It relies on select * and the columns can be whatever. There are already CLI means of doing random queries against whatever columns. AMI doesn't seem like that much of a stretch.


> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, lines 141-161
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line141>
> >
> >     Without a column name that contains a single quote character, this is also superfluous.

See above.


> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, lines 878-880
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line878>
> >
> >     The last type may not work well for type INTEGER, because integers are converted internally to signed 8-byte integers.  Any part of the unsigned value will therefore be converted to negative or worse, truncated.  Nothing currently in Asterisk uses that type, yet, so it's not a big concern, but RQ_UINTEGER8 should probably be treated as a "TEXT" anyway, for future compatibility.

There are no types in sqlite3 anyway, just type affinity. You can store any kind of data in any kind of field in sqlite. You can store "Tilghman was here" in an integer column. The sqlite3_exec callback will return everything back as a text field anyway.


sqlite> CREATE TABLE sipregs (name TEXT, ipaddr TEXT, port INTEGER, regseconds INTEGER, defaultuser TEXT, fullcontact TEXT, regserver TEXT, useragent TEXT, lastms INTEGER);
sqlite> insert into sipregs (lastms) values ("hello");
sqlite> select * from sipregs;
||||||||hello


> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, line 998
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line998>
> >
> >     isigned?  Should that be signed or unsigned?

Oops, signed. I'll fix that. :-)


> On Sept. 5, 2011, 2:11 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, lines 131-134
> > <https://reviewboard.asterisk.org/r/1408/diff/3/?file=19946#file19946line131>
> >
> >     Why escape the table name at all?  I think it's more logical to simply disallow badly named tables, instead of trying to handle them.  Additionally, it leads to a false sense of security, because the escape function in use only deals with single quote characters.  Supposing somebody put a double quote character in a table name specification!  Of course, this also requires admin access (to specify a table name in the config) and we cannot always protect admins from themselves.

Actually, I should have used %w to handle the double quote issue (and probably should do that for column names), but since tables have to be defined in the config file to match--even if we opened up a generic AMI interface to realtime, we could just drop trying to escape them.


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1408/#review4227
-----------------------------------------------------------


On Sept. 3, 2011, 11:08 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1408/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2011, 11:08 p.m.)
> 
> 
> Review request for Asterisk Developers, Olle E Johansson and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> This patch adds an SQLite 3 realtime driver. It supports multiple databases, ast_realtime_require (warn, createclose, and createchar support including the ability to create missing tables), static realtime, and batching commits in transactions to increase write performance. It currently caches no table data, but I figure that premature optimization is bad anyway.
> 
> It, like other realtime drivers, doesn't escape data when building the SQL strings. I started out using parameter binding, but SQLite doesn't let you bind column names, so given how dynamic everything with realtime is, I just when with building the statements outright. I could manually escape the parameters, but the only thing in the SQLite 3 library I could find that does that dynamically allocates the string and that ends up being a lot of little allocs and frees. I suppose we could write our own using a single ast_str with some ast_str_reset calls between each one, but it is a bit of a pain. Until we want to open up the realtime api to things like AMI where user-generated input is going to be a problem, escaping isn't really necessary.
> 
> The impetus for writing this is that Asterisk now has a built-in SQLite 3 database which the astdb uses. The next step would be to convert the astdb to using the realtime calls, thus making it possible for it to be used with any realtime backend (but using the default SQLite 3 db unless configured otherwise). After that, we can start rewriting some things that use the astdb in very crude ways to start using realtime calls to the internal (or external, if configured that way) databases so that the data can be stored in more logical ways than a key/value store allows.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_config_sqlite3.c PRE-CREATION 
>   /trunk/configs/res_config_sqlite3.conf.sample PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1408/diff
> 
> 
> Testing
> -------
> 
> Verified that static realtime extensions.conf loads properly. Realtime CLI commands all behave as expected. Tested unload/load and reload scenarios making sure that they behaved as expected and that there were no ref/memory leaks. Made sure that batching worked.
> 
> Once we move the astdb to using realtime calls, the existing unit tests for that should give a workout to the realtime driver as well. I didn't write a new test pointing to the astdb because the astdb locks its database so it cannot be written to from another connection.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110905/445e015d/attachment-0001.htm>