sqlite module has bad argument parsing code, including undefined behavior in C

[larryhastings]

BPO	20274
Nosy	@birkenfeld, @larryhastings, @benjaminp, @bitdancer, @serhiy-storchaka

^{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/larryhastings'
closed_at = <Date 2015-05-08.14:47:17.800>
created_at = <Date 2014-01-15.20:24:43.726>
labels = ['type-bug']
title = 'sqlite module has bad argument parsing code, including undefined behavior in C'
updated_at = <Date 2015-05-08.16:56:56.513>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2015-05-08.16:56:56.513>
actor = 'larry'
assignee = 'larry'
closed = True
closed_date = <Date 2015-05-08.14:47:17.800>
closer = 'larry'
components = []
creation = <Date 2014-01-15.20:24:43.726>
creator = 'larry'
dependencies = []
files = []
hgrepos = []
issue_num = 20274
keywords = []
message_count = 12.0
messages = ['208189', '208204', '208224', '242766', '242768', '242769', '242770', '242771', '242772', '242774', '242775', '242776']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'ghaering', 'larry', 'benjamin.peterson', 'r.david.murray', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue20274'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[larryhastings]

The code in Modules/_sqlite/connection.c is sloppy.

The functions pysqlite_connection_execute, pysqlite_connection_executemany, and pysqlite_connection_executescript accept a third "PyObject *kwargs". However none of these functions are marked METH_KEYWORD. This only works because the kwargs parameter is actually ignored--the functions only support positional-only arguments. Obviously the "PyObject *kwargs" parameters should be removed for these three functions.

A slightly more advanced problem: pysqlite_connection_call, which implements sqlite3.Connection.__call__(), ignores its kwargs parameter completely. If it doesn't accept keyword parameters it should at least complain if any are passed in.

Georg: you want this fixed in 3.3? 3.2?
Benjamin: you want this fixed in 2.7?

[bitdancer]

Why do you want to fix it in order versions? Can it lead to a crash?

For __call__, it seems to me we should do a deprecation and remove it in 3.5. Otherwise we'll risk breaking working code for no good reason (working code with useless parameters, but still working code :)

[larryhastings]

You're right, deprecation sounds best. If Georg or Benjamin want the fix in earlier releases I'll split the pysqlite_connection_call into another issue, otherwise I won't bother.

As for fixing the undefined behavior in older versions: since its behavior is undefined, yes, it *could* cause a crash. But by that token it *could* teleport you to Mars and give you a funny-looking nose. In practice it *should* be utterly harmless, as I believe every platform supported by Python 3.4 uses the caller-pops-stack calling convention. And we've lived with it for this long, so it doesn't seem to be hurting anything. But like all undefined behavior I can make no guarantee.

MvL in particular comes down like a ton of bricks whenever someone proposes checking in code that's technically undefined behavior. I've had the relevant chapter and verse of the C standard quoted at me for this exact thing (calling a function pointer using a sliiiightly different signature than the actual function).

[larryhastings]

I'm gonna fix this now. (I'm cleaning up some old issues I filed on the bug tracker this morning.)

For 3.4, I'm just removing the PyObject *kwargs for those three functions that don't actually accept keyword arguments (METH_VARARGS) and aren't passed that parameter. That's a bug plain and simple, it's relying on undefined behavior, and it's better that we fix it.

For 3.5. I'm adding a call to _PyArg_NoKeywords() to pysqlite_connection_call. Previously it simply ignored any/all keyword arguments; now it will complain if it is passed any. We don't need a deprecation cycle for that.

[python-dev]

New changeset 4c860369b6c2 by Larry Hastings in branch '3.4':
Issue bpo-20274: Remove ignored and erroneous "kwargs" parameters from three
https://hg.python.org/cpython/rev/4c860369b6c2

New changeset 3e9f4f3c7fa7 by Larry Hastings in branch 'default':
Issue bpo-20274: When calling a _sqlite.Connection, it now complains if passed
https://hg.python.org/cpython/rev/3e9f4f3c7fa7

[serhiy-storchaka]

Is 2.7 affected?

[larryhastings]

Yes, all those bugs exist in 2.7. However, Benjamin hasn't responded to this bug, so I assume he doesn't care and I should leave 2.7 alone.

[serhiy-storchaka]

I think the bug should be fixed in 2.7 (but not in 3.3 unless we get a crasher).

[benjaminp]

I don't mind if you fix it in 2.7, too.

(Sorry, I get a lot of bug related emails...)

[larryhastings]

Benjamin: I assume you want the extraneous (and undefined behavior) kwargs parameters removed.

Do you also want pysqlite_connection_call() to start calling _PyArg_NoKeywords()?

[benjaminp]

On Fri, May 8, 2015, at 12:08, Larry Hastings wrote:

Larry Hastings added the comment:

Benjamin: I assume you want the extraneous (and undefined behavior)
kwargs parameters removed.

Do you also want pysqlite_connection_call() to start calling
_PyArg_NoKeywords()?

Yes, please.

[python-dev]

New changeset c91d135b0776 by Larry Hastings in branch '2.7':
Issue bpo-20274: When calling a _sqlite.Connection, it now complains if passed
https://hg.python.org/cpython/rev/c91d135b0776