Skip to content

Commit

Permalink
SQLite driver: fix crash when binding a QByteArray/QString
Browse files Browse the repository at this point in the history
Passing SQLITE_STATIC to sqlite3_bind_*() means that ownership
of the data stays in the caller, i.e. SQLite itself doesn't make a copy;
such data must be therefore be kept valid until sqlite3_step() is called.

The code in the SQLite driver uses that option to avoid copying byte
array or string data. But, unlike what the comments in the code say, we
do NOT keep the QByteArray/QString alive long enough: they're contained
by a temporary QVariant object which gets destroyed at the end of the
loop that binds each argument.

Luckily the fix is simple: since that QVariant is just a copy of the
QVariants used as bound parameters, and these are held in a container
(which lives long enough), simply create a reference to the container's
elements rather than a copy. This ensures that the data is alive by
the time sqlite3_step() is called.

This problem doesn't normally appear because of implicit sharing of
QByteArray/QString. When the QVariant is copied, the inner element
is just a shallow copy. Getting the pointer to the data, and destroying
the QVariant, does not destroy the data (it's kept alive by the
QByteArray/QString inside the *copied-from* QVariant).

Of course there's a catch: if the *copied-from* QVariant contains a
QString created via fromRawData, then everything blows up. In this case,

1. the copied QVariant is created (which bumps the QString refcount)¹
2. the QString inside of it is accessed directly (via
QVariant::constData)
3. utf16() is called on that string, which detaches it (!)
4. the result of utf16() is passed to SQLite, with SQLITE_STATIC
5. the copied QVariant is destroyed; this destroys the inner QString,
which, being detached, deallocates the data too early.
6. sqlite3_step() is called, kaboom.

(The copied-from QVariant still has the string created by fromRawData.)

¹ Note that QString uses the Small QVariant Optimization, so the QString
object itself into the QVariant is copied, it's not just a *QVariant*
refcount increase.

Change-Id: Idcdb192809f1f8f79b4a901e1247f933eb06e854
Pick-to: 6.1 5.15 5.12
Fixes: QTBUG-94070
Reviewed-by: Andy Shaw <[email protected]>
  • Loading branch information
dangelog committed May 31, 2021
1 parent 459529a commit 0f38259
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/plugins/sqldrivers/sqlite/qsql_sqlite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ bool QSQLiteResult::exec()
if (paramCountIsValid) {
for (int i = 0; i < paramCount; ++i) {
res = SQLITE_OK;
const QVariant value = values.at(i);
const QVariant &value = values.at(i);

if (value.isNull()) {
res = sqlite3_bind_null(d->stmt, i + 1);
Expand Down

0 comments on commit 0f38259

Please sign in to comment.