classification
Title: sqlite module has bad argument parsing code, including undefined behavior in C
Type: behavior Stage: resolved
Components: Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: benjamin.peterson, georg.brandl, ghaering, larry, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords:

Created on 2014-01-15 20:24 by larry, last changed 2015-05-08 16:56 by larry. This issue is now closed.

Messages (12)
msg208189 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-15 20:24
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?
msg208204 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-01-15 22:21
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 :)
msg208224 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-16 00:06
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).
msg242766 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-08 14:25
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.
msg242768 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-08 14:45
New changeset 4c860369b6c2 by Larry Hastings in branch '3.4':
Issue #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 #20274: When calling a _sqlite.Connection, it now complains if passed
https://hg.python.org/cpython/rev/3e9f4f3c7fa7
msg242769 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-08 14:47
Is 2.7 affected?
msg242770 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-08 14:49
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.
msg242771 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-08 15:01
I think the bug should be fixed in 2.7 (but not in 3.3 unless we get a crasher).
msg242772 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-05-08 15:11
I don't mind if you fix it in 2.7, too.

(Sorry, I get a lot of bug related emails...)
msg242774 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-08 16:08
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()?
msg242775 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-05-08 16:40
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.
msg242776 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-08 16:56
New changeset c91d135b0776 by Larry Hastings in branch '2.7':
Issue #20274: When calling a _sqlite.Connection, it now complains if passed
https://hg.python.org/cpython/rev/c91d135b0776
History
Date User Action Args
2015-05-08 16:56:56larrysetversions: + Python 2.7
2015-05-08 16:56:50python-devsetmessages: + msg242776
2015-05-08 16:40:01benjamin.petersonsetmessages: + msg242775
2015-05-08 16:08:02larrysetmessages: + msg242774
2015-05-08 15:11:16benjamin.petersonsetmessages: + msg242772
2015-05-08 15:01:47serhiy.storchakasetmessages: + msg242771
2015-05-08 14:49:32larrysetversions: + Python 3.5
2015-05-08 14:49:27larrysetmessages: + msg242770
2015-05-08 14:47:48serhiy.storchakasetmessages: + msg242769
2015-05-08 14:47:17larrysetstatus: open -> closed
assignee: ghaering -> larry
resolution: fixed
stage: needs patch -> resolved
2015-05-08 14:45:28python-devsetnosy: + python-dev
messages: + msg242768
2015-05-08 14:25:22larrysetmessages: + msg242766
2015-01-11 02:03:26ghaeringsetassignee: ghaering

nosy: + ghaering
2014-01-16 00:06:57larrysetmessages: + msg208224
2014-01-15 22:21:25r.david.murraysetnosy: + r.david.murray
messages: + msg208204
2014-01-15 20:26:18serhiy.storchakasetnosy: + serhiy.storchaka
2014-01-15 20:24:43larrycreate