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

Jump to bottom

[sqlite3] sqlite3_column_name() failures should raise MemoryError #87417

Closed
erlend-aasland opened this issue Feb 18, 2021 · 15 comments
Closed

[sqlite3] sqlite3_column_name() failures should raise MemoryError #87417

erlend-aasland opened this issue Feb 18, 2021 · 15 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@erlend-aasland
Copy link
Contributor

BPO 43251
Nosy @berkerpeksag, @serhiy-storchaka, @erlend-aasland
PRs
  • bpo-43251: sqlite3_column_name() failures now raise MemoryError #24609
    		•

    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 = None
closed_at = <Date 2021-02-28.17:01:29.187>
created_at = <Date 2021-02-18.11:22:34.286>
labels = ['type-feature', 'library', '3.10']
title = '[sqlite3] sqlite3_column_name() failures should raise MemoryError'
updated_at = <Date 2021-02-28.17:01:29.186>
user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2021-02-28.17:01:29.186>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2021-02-28.17:01:29.187>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2021-02-18.11:22:34.286>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 43251
keywords = ['patch']
message_count = 15.0
messages = ['387214', '387336', '387347', '387402', '387403', '387405', '387428', '387446', '387447', '387565', '387566', '387567', '387591', '387620', '387810']
nosy_count = 3.0
nosy_names = ['berker.peksag', 'serhiy.storchaka', 'erlendaasland']
pr_nums = ['24609']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue43251'
versions = ['Python 3.10']
    @erlend-aasland
    Copy link
    Contributor Author

    AFAICS, the docs for sqlite3_column_name() says that if it returns NULL, we are out of memory, thus we should call PyErr_NoMemory() and bail.

    FYI, there are the calls to sqlite3_column_name() in Modules/_sqlite//cursor.c

    Ref:

    @erlend-aasland erlend-aasland added 3.10 only security fixes stdlib Python modules in the Lib dir labels Feb 18, 2021
    @erlend-aasland erlend-aasland changed the title sqlite3_column_name() failures should call PyErr_NoMemory() sqlite3_column_name() failures should raise MemoryError Feb 19, 2021
    @erlend-aasland erlend-aasland changed the title sqlite3_column_name() failures should call PyErr_NoMemory() sqlite3_column_name() failures should raise MemoryError Feb 19, 2021
    @serhiy-storchaka
    Copy link
    Member

    Well, it returns NULL in case of out of memory, but is it the only cause? Can NULL be returned for other reasons?

    @erlend-aasland
    Copy link
    Contributor Author

    Well, it returns NULL in case of out of memory, but is it the only cause? Can NULL be returned for other reasons?

    According to the SQLite docs, no. Looking at the source code, we see that it also returns NULL if the second parameter (column index) is out of range, but we already check that. AFAICS, the only reason for a NULL in our case is OOM.

    @erlend-aasland erlend-aasland changed the title sqlite3_column_name() failures should raise MemoryError [sqlite3] sqlite3_column_name() failures should raise MemoryError Feb 19, 2021
    @erlend-aasland erlend-aasland changed the title sqlite3_column_name() failures should raise MemoryError [sqlite3] sqlite3_column_name() failures should raise MemoryError Feb 19, 2021
    @serhiy-storchaka
    Copy link
    Member

    It also returns NULL if the column name was not set (is it even possible?).

    @erlend-aasland
    Copy link
    Contributor Author

    That's inside sqlite3_value_text() and friends, then? Let's investigate further before concluding.

    @serhiy-storchaka
    Copy link
    Member

    Investigate where column names are initialized and whether is it possible to get them NULLs. The array of column names is filled with NULLs when initialized and later it is filled with references to strings. I am wondering whether there is a gap between this which lefts some column names as NULLs.

    @erlend-aasland
    Copy link
    Contributor Author

    "SELECT 1" yields "1" as column name.
    "SELECT <...> AS N" yields "N" as column name.
    "SELECT NULL" yields "NULL" as column name.

    We can't set the column name via the API, so that's pretty much it (implicit name or explicit through the AS keyword). As long as sqlite3_prepare_v2() returned SQLITE_OK, it seems that SQLite assures we've got a valid column name.

    As far as I can sqlite3_column_name() returns NULL if:

    1. The first parameter (stmt) is NULL
    2. The second parameter is out of bounds
    3. Memory error
    4. No column name is set (SQLite bug)

    We've got 1) and 2) covered. SQLite has a pretty good code coverage, so I'd say 4) is unlikely.

    What do you think?

    @serhiy-storchaka
    Copy link
    Member

    Please go ahead!

    @erlend-aasland
    Copy link
    Contributor Author

    Will do. Thanks for pushing the investigation.

    @erlend-aasland
    Copy link
    Contributor Author

    After discussing the matter briefly on the SQLite forum, I'm no longer 100% certain about this. There seems to be uncertainty about which other conditions can produce NULL, although memory error seems to be the most probable. I consider closing both this issue and the PR and just leave the code as it is.

    @serhiy-storchaka
    Copy link
    Member

    Could you please give a link to the discussion?

    @erlend-aasland
    Copy link
    Contributor Author

    Sure! Here it is: https://sqlite.org/forum/forumpost/574c6bc66c

    @erlend-aasland
    Copy link
    Contributor Author

    Larry Brasfield's comment https://sqlite.org/forum/forumpost/6430fc589d?t=h aligns with https://bugs.python.org/issue43251#msg387428

    I'll think twice before posting there again, though.

    @erlend-aasland
    Copy link
    Contributor Author

    I believe we can proceed with this as planned. Serhiy, do you have additional comments or change requests?

    @berkerpeksag
    Copy link
    Member

    New changeset 2183d06 by Erlend Egeberg Aasland in branch 'master':
    bpo-43251: sqlite3_column_name() failures now raise MemoryError (GH-24609)
    2183d06

    @berkerpeksag berkerpeksag added the type-feature A feature request or enhancement label Feb 28, 2021
    @berkerpeksag berkerpeksag added the type-feature A feature request or enhancement label Feb 28, 2021
    @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
    Assignees
    No one assigned
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @berkerpeksag @serhiy-storchaka @erlend-aasland