sqlite3 type confusion and multiple frees

[tehybel]

BPO	27861
Nosy	@berkerpeksag, @serhiy-storchaka, @zhangyangyu
Files	issue27861.patch issue27861_conn_cursor.patch issue27861_conn_isolation_level.patch issue27861_conn_cursor_v2.patch issue27861_conn_cursor_v3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-08-30.10:51:58.412>
created_at = <Date 2016-08-25.15:36:51.285>
labels = ['extension-modules', 'type-crash']
title = 'sqlite3 type confusion and multiple frees'
updated_at = <Date 2016-08-30.10:51:58.411>
user = 'https://bugs.python.org/tehybel'

bugs.python.org fields:

activity = <Date 2016-08-30.10:51:58.411>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-08-30.10:51:58.412>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2016-08-25.15:36:51.285>
creator = 'tehybel'
dependencies = []
files = ['44227', '44228', '44234', '44241', '44245']
hgrepos = []
issue_num = 27861
keywords = ['patch']
message_count = 14.0
messages = ['273659', '273696', '273700', '273701', '273704', '273707', '273771', '273787', '273789', '273794', '273808', '273810', '273811', '273855']
nosy_count = 6.0
nosy_names = ['ghaering', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang', 'tehybel']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue27861'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[tehybel]

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
...

[zhangyangyu]

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.

bpo-27861 tries to fix this. Besides, it also alters Connection.cursor's doc since it now does not reflects the implementation.

[berkerpeksag]

Please split bpo-27861.patch into two patches.

It would be better if isolation_level wouldn't accept values other than None, "" (empty string), "deferred", "immediate", "exclusive".

[zhangyangyu]

I considered that but don't why decide to preserve it. But now that you mention it, I'll do that later.

[zhangyangyu]

issue27861_conn_cursor.patch tries to solve the first issue.

[zhangyangyu]

issue27861_conn_isolation_level.patch now adds argument type and value check along with eliminating the potential sf.

[serhiy-storchaka]

Agreed, there are two unrelated bugs. It would be better to discuss separate patches in separate issues.

[zhangyangyu]

Thanks for the comments, Serhiy and palaviv. I left some questions in reply to them.

[serhiy-storchaka]

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.

[zhangyangyu]

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.

[zhangyangyu]

Thanks for the comments Serhiy. v3 now fixs all that.

[serhiy-storchaka]

LGTM except one style nit.

[zhangyangyu]

It's a trivial personal style choice I think. Do what you want if you'd like to merge it.