Open Bug 1514683 Opened Last year Updated 10 months ago
Evaluate enabling SQLITE
SQLITE_DBCONFIG_DEFENSIVE is a new flag that disallows a few dangerous features of Sqlite that may corrupt databases. We surely can take this for our products, my only concern is that we may end up forcing this on system Sqlite. It doesn't seem particularly bad but it will disallow those features more widely.
A possible take on this may be that system sqlite should be safer, while apps willing to expose dangerous features should use their own compile, rather than the opposite.
As noted in the previous bug, this change passes in our CI. So like you said, it seems like the main unresolved question is what to do with downstream distros.
Imho we should take this, my opinion is in comment 1. It's worth getting more opinions though, Andrew, what do you think?
I also think we should take this. In terms of system SQLite, I think that one of the main arguments of using system packaged SQLite is that they're kept up-to-date for security reasons. I don't think there's any sane/safe reason to not want SQLITE_DBCONFIG_DEFENSIVE, so I think we're aligned with disto's here. In the event a distro doesn't want it, they can patch out the configure check which is our only enforcement here.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/7408e4b312e9 Enable SQLITE_DBCONFIG_DEFENSIVE. r=mak
I don’t think this patch was correct. From my reading of the SQLite source, SQLITE_DBCONFIG_DEFENSIVE isn’t a compile time option, but rather a bitflag that can be supplied to SQLite at runtime when establishing a connection. As a result of this, I can no longer build Firefox Nightly from source, even if I build SQLite with -DSQLITE_DBCONFIG_DEFENSIVE. It’s possible -DSQLITE_DBCONFIG_DEFENSIVE=1 will build siccessfully (I’m trying that now), but I still don’t think it will do the right thing. If I’m correct, and SQLITE_DBCONFIG_DEFENSIVE isn’t a compile-time option to SQLite, this change should be reverted, and then as a follow up, Firefox can take advantage of this flag by passing it when it creates a SQLite connection.
Indeed, thanks for pointing it out!
Backout by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/079be36cc55e Backed out changeset 7408e4b312e9 because it's not the correct way to enable SQLITE_DBCONFIG_DEFENSIVE.
Setting connection-time options isn't something I'm familiar with - can you please point me in the right direction, Marco?
SQLITE_DBCONFIG_* options are set with sqlite3_db_config. https://www.sqlite.org/c3ref/c_dbconfig_defensive.html
oops, this is totally my fault, I missed the DBCONFIG bit :( This is even better, we won't touch system sqlite. You can have a look at the patch in bug 1270882 that, modulo the define we don't need here, does pretty much the same (also, check my review comment on it). The only difference is that, based on the documentation, SQLITE_DBCONFIG_DEFENSIVE doesn't have any additional argument, so it should be enough to: srv = ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_DEFENSIVE); MOZ_ASSERT(srv == SQLITE_OK, "SQLITE_DBCONFIG_DEFENSIVE should be enabled"); in the same call points
Attachment #9032134 - Attachment is obsolete: true
Flags: needinfo?(mak77) → needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.