Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
larryhastings opened this issue Jan 15, 2014 · 12 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@larryhastings
Copy link
Contributor

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
Copy link
Contributor Author

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?

@larryhastings larryhastings added the type-bug An unexpected behavior, bug, or error label Jan 15, 2014
@bitdancer
Copy link
Member

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
Copy link
Contributor Author

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

@ghaering ghaering mannequin self-assigned this Jan 11, 2015
@larryhastings
Copy link
Contributor Author

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
Copy link
Mannequin

python-dev mannequin commented May 8, 2015

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
Copy link
Member

Is 2.7 affected?

@larryhastings
Copy link
Contributor Author

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
Copy link
Member

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

@benjaminp
Copy link
Contributor

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

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

@larryhastings
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Mannequin

python-dev mannequin commented May 8, 2015

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

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants