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

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?

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 :)

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

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.

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

Is 2.7 affected?

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.

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

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

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

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()?

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.

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

nosy: + ghaering