classification
Title: sqlite3 type confusion and multiple frees
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: berker.peksag, ghaering, python-dev, serhiy.storchaka, tehybel, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-08-25 15:36 by tehybel, last changed 2016-08-30 10:51 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
issue27861.patch xiang.zhang, 2016-08-26 06:12 review
issue27861_conn_cursor.patch xiang.zhang, 2016-08-26 12:40 review
issue27861_conn_isolation_level.patch xiang.zhang, 2016-08-26 14:35 review
issue27861_conn_cursor_v2.patch xiang.zhang, 2016-08-27 19:14 review
issue27861_conn_cursor_v3.patch xiang.zhang, 2016-08-28 05:52 review
Messages (14)
msg273659 - (view) Author: tehybel (tehybel) Date: 2016-08-25 15:36
The first issue is a type confusion which resides in the sqlite3 module, in the
file connection.c. The function pysqlite_connection_cursor takes an optional
argument, a factory callable:

    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|O", kwlist,
                                     &factory)) {
        return NULL;
    }

If the factory callable is given, it is called to initialize a cursor:

    cursor = PyObject_CallFunction(factory, "O", self);

After this the cursor, which is a PyObject *, is cast directly to a
pysqlite_Cursor without performing any type checking:

    if (cursor && self->row_factory != Py_None) {
        Py_INCREF(self->row_factory);
        Py_XSETREF(((pysqlite_Cursor *)cursor)->row_factory, self->row_factory);
    }

Here is a small script which is tested on Python-3.5.2, 64-bit, with
--with-pydebug enabled:

--- begin script ---

import sqlite3

conn = sqlite3.connect('poc2.db')
conn.row_factory = 12
conn.cursor(lambda x: "A"*0x10000)

--- end script ---


When run, this produces a segfault:

(gdb) r ./poc2.py

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6496ad8 in pysqlite_connection_cursor (self=0x7ffff68cc370, args=<optimized out>, kwargs=<optimized out>)
    at /home/xx/Python-3.5.2/Modules/_sqlite/connection.c:322
warning: Source file is more recent than executable.
322         Py_XSETREF(((pysqlite_Cursor *)cursor)->row_factory, self->row_factory);
(gdb) p cursor
$13 = (PyObject *) 0xa46b90
(gdb) p self->row_factory
$14 = (PyObject *) 0x8d05f0 <small_ints+816>
(gdb) x/3i $pc
=> 0x7ffff6496ad8 <pysqlite_connection_cursor+221>: mov    rax,QWORD PTR [rdi+0x10]
   0x7ffff6496adc <pysqlite_connection_cursor+225>: sub    rax,0x1
   0x7ffff6496ae0 <pysqlite_connection_cursor+229>: mov    QWORD PTR [rdi+0x10],rax
(gdb) p $rdi
$15 = 0x4141414141414141

An arbitrary word in memory is decremented.


------


The second issue exists in the function pysqlite_connection_set_isolation_level
which resides in /Modules/_sqlite/connection.c. It can result in memory getting
freed multiple times.

The problem is that the variable self->isolation_level is not cleared before
being DECREF'd.

The code looks like this:

    static int pysqlite_connection_set_isolation_level(pysqlite_Connection* self, PyObject* isolation_level)
    {
        ...
        Py_XDECREF(self->isolation_level);
        ...
    }

This call to Py_XDECREF can trigger an arbitrary amount of python code, e.g. via
self->isolation_level's __del__ method. That code could then call 
pysqlite_connection_set_isolation_level again, which would trigger another
Py_XDECREF call on the same self->isolation_level, which can thus be freed an
arbitrary number of times.

One way to fix this is to use Py_CLEAR instead.

Here's a proof-of-concept script which results in a segfault here:

--- begin script ---

import sqlite3

class S(str):
    def __del__(self):
        conn.isolation_level = S("B")

conn = sqlite3.connect('poc6.db')
conn.isolation_level = S("A")
conn.isolation_level = ""

--- end script ---

When run it segfaults here, with Python-3.5.2 and --with-pydebug enabled:

(gdb) r ./poc6.py
Starting program: /home/xx/Python-3.5.2/python ./poc6.py

Program received signal SIGSEGV, Segmentation fault.
_Py_ForgetReference (op=op@entry=0x7ffff6d81b80) at Objects/object.c:1757
1757        if (op == &refchain ||
(gdb) bt
#0  _Py_ForgetReference (op=op@entry=0x7ffff6d81b80) at Objects/object.c:1757
#1  0x000000000049f8c0 in _Py_Dealloc (op=0x7ffff6d81b80) at Objects/object.c:1785
#2  0x000000000046ced8 in method_dealloc (im=im@entry=0x7ffff7f25de8) at Objects/classobject.c:198
#3  0x000000000049f8c5 in _Py_Dealloc (op=op@entry=0x7ffff7f25de8) at Objects/object.c:1786
...
msg273696 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-26 06:12
For the first issue, the doc says:

The cursor method accepts a single optional parameter cursorClass. If supplied, this must be a custom cursor class that extends sqlite3.Cursor.

So I think we should check the type and then raise TypeError.

For the second issue, it seems it just hits the Py_CLEAR comments.

issue27861 tries to fix this. Besides, it also alters Connection.cursor's doc since it now does not reflects the implementation.
msg273700 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-26 10:51
Please split issue27861.patch into two patches.

It would be better if isolation_level wouldn't accept values other than None, "" (empty string), "deferred", "immediate", "exclusive".
msg273701 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-26 11:15
I considered that but don't why decide to preserve it. But now that you mention it, I'll do that later.
msg273704 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-26 12:40
issue27861_conn_cursor.patch tries to solve the first issue.
msg273707 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-26 14:35
issue27861_conn_isolation_level.patch now adds argument type and value check along with eliminating the potential sf.
msg273771 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-27 10:04
Agreed, there are two unrelated bugs. It would be better to discuss separate patches in separate issues.
msg273787 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-27 16:56
Thanks for the comments, Serhiy and palaviv. I left some questions in reply to them.
msg273789 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-27 18:02
About the first bug. Checking that a factory is a subclass of sqlite3.Cursor is not enough. I suppose that a subclass of sqlite3.Cursor with overridden __new__ could pass the check but cause a crash.

    class BadCursor(sqlite3.Cursor):
        def __new__(cls, conn):
            return None

Docs should be changed as well as the code.

As for the second bug, please open a separate issue. This is more complex issue.
msg273794 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-27 19:14
If decide to follow the behaviour of code rather than doc, it's a different story. issue27861_conn_cursor_v2.patch tries to solve the first issue. It now preserve the previous behaviour treating factory as a callable not a cursor type.
msg273808 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-28 05:52
Thanks for the comments Serhiy. v3 now fixs all that.
msg273810 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-28 07:02
LGTM except one style nit.
msg273811 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-28 07:08
It's a trivial personal style choice I think. Do what you want if you'd like to merge it.
msg273855 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-29 12:12
New changeset c046cbb24f98 by Serhiy Storchaka in branch '3.5':
Issue #27861: Fixed a crash in sqlite3.Connection.cursor() when a factory
https://hg.python.org/cpython/rev/c046cbb24f98

New changeset 97dbba8a6d4a by Serhiy Storchaka in branch '2.7':
Issue #27861: Fixed a crash in sqlite3.Connection.cursor() when a factory
https://hg.python.org/cpython/rev/97dbba8a6d4a

New changeset afcec2d11e9e by Serhiy Storchaka in branch 'default':
Issue #27861: Fixed a crash in sqlite3.Connection.cursor() when a factory
https://hg.python.org/cpython/rev/afcec2d11e9e
History
Date User Action Args
2016-08-30 10:51:58serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-08-29 12:12:31python-devsetnosy: + python-dev
messages: + msg273855
2016-08-29 11:19:52serhiy.storchakasetassignee: serhiy.storchaka
2016-08-28 07:08:37xiang.zhangsetmessages: + msg273811
2016-08-28 07:02:30serhiy.storchakasetmessages: + msg273810
2016-08-28 05:52:16xiang.zhangsetfiles: + issue27861_conn_cursor_v3.patch

messages: + msg273808
2016-08-27 19:14:51xiang.zhangsetfiles: + issue27861_conn_cursor_v2.patch

messages: + msg273794
2016-08-27 18:02:55serhiy.storchakasetmessages: + msg273789
2016-08-27 16:56:46xiang.zhangsetmessages: + msg273787
2016-08-27 10:04:30serhiy.storchakasetmessages: + msg273771
2016-08-26 14:35:42xiang.zhangsetfiles: + issue27861_conn_isolation_level.patch

messages: + msg273707
2016-08-26 12:40:10xiang.zhangsetfiles: + issue27861_conn_cursor.patch

messages: + msg273704
2016-08-26 11:15:03xiang.zhangsetmessages: + msg273701
2016-08-26 10:51:10berker.peksagsetmessages: + msg273700
stage: needs patch -> patch review
2016-08-26 06:12:36xiang.zhangsetfiles: + issue27861.patch

nosy: + xiang.zhang
messages: + msg273696

keywords: + patch
2016-08-25 16:32:50serhiy.storchakasetnosy: + serhiy.storchaka
2016-08-25 15:40:57berker.peksagsetnosy: + berker.peksag
stage: needs patch
type: crash

versions: + Python 3.6
2016-08-25 15:36:51tehybelcreate