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

[sqlite3] Improve sqlite3_value_text() error handling #88019

Closed
erlend-aasland opened this issue Apr 15, 2021 · 17 comments
Closed

[sqlite3] Improve sqlite3_value_text() error handling #88019

erlend-aasland opened this issue Apr 15, 2021 · 17 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@erlend-aasland
Copy link
Contributor

BPO 43853
Nosy @gvanrossum, @berkerpeksag, @serhiy-storchaka, @JelleZijlstra, @pablogsal, @miss-islington, @erlend-aasland
PRs
  • bpo-43853: Handle sqlite3_value_text() errors #25422
  • [3.10] bpo-43853: Handle sqlite3_value_text() errors (GH-25422) #26534
  • [3.9] bpo-43853: Handle sqlite3_value_text() errors (GH-25422). #27627
  • bpo-43853: Expand test suite for SQLite UDF's #27642
  • bpo-43853: Amend NEWS entry #27922
  • [3.9] bpo-43853: Amend NEWS entry for latest changes in sqlite3 (GH-27922) #27952
  • [3.10] bpo-43853: Amend NEWS entry for latest changes in sqlite3 (GH-27922) #27953
  • [3.10] bpo-43853: Expand test suite for SQLite UDF's (GH-27642) #31030
  • [3.9] bpo-43853: Expand test suite for SQLite UDF's (GH-27642) (GH-31030) #31586
  • Files
  • patch.diff
  • 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 2022-03-02.06:41:01.374>
    created_at = <Date 2021-04-15.10:00:09.823>
    labels = ['extension-modules', 'type-bug', '3.9', '3.10', '3.11']
    title = '[sqlite3] Improve sqlite3_value_text() error handling'
    updated_at = <Date 2022-03-02.06:41:01.373>
    user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2022-03-02.06:41:01.373>
    actor = 'erlendaasland'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-03-02.06:41:01.374>
    closer = 'erlendaasland'
    components = ['Extension Modules']
    creation = <Date 2021-04-15.10:00:09.823>
    creator = 'erlendaasland'
    dependencies = []
    files = ['49961']
    hgrepos = []
    issue_num = 43853
    keywords = ['patch']
    message_count = 17.0
    messages = ['391124', '394267', '395109', '395112', '399075', '399084', '399114', '399120', '400174', '400288', '400292', '400296', '400319', '401565', '411771', '413620', '414325']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'berker.peksag', 'serhiy.storchaka', 'JelleZijlstra', 'pablogsal', 'miss-islington', 'erlendaasland']
    pr_nums = ['25422', '26534', '27627', '27642', '27922', '27952', '27953', '31030', '31586']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue43853'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @erlend-aasland
    Copy link
    Contributor Author

    Fix sqlite3_value_text() usage:

    • Raise MemoryError if sqlite3_value_text() sets SQLITE_NOMEM
    • Let PyUnicode_FromStringAndSize() errors propagate

    Quoting the SQLite docs:
    "As long as the input parameter is correct, these routines can only fail if an out-of-memory error occurs"

    See also:

    @erlend-aasland erlend-aasland added 3.10 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Apr 15, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite] Fix sqlite3_value_text() usage [sqlite3] Fix sqlite3_value_text() usage Apr 15, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite] Fix sqlite3_value_text() usage [sqlite3] Fix sqlite3_value_text() usage Apr 15, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite3] Fix sqlite3_value_text() usage [sqlite3] Improve sqlite3_value_text() error handling May 9, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite3] Fix sqlite3_value_text() usage [sqlite3] Improve sqlite3_value_text() error handling May 9, 2021
    @erlend-aasland
    Copy link
    Contributor Author

    The affected branch is exercised by the following tests:

    • test_aggr_check_param_blob
    • test_aggr_check_param_float
    • test_aggr_check_param_int
    • test_aggr_check_param_none
    • test_aggr_check_param_str
    • test_aggr_check_params_int
    • test_aggr_exception_in_finalize
    • test_aggr_exception_in_step
    • test_aggr_no_finalize
    • test_param_string

    I've expanded test_aggr_check_param_str and test_param_string to also check empty strings.

    @pablogsal
    Copy link
    Member

    New changeset 006fd86 by Erlend Egeberg Aasland in branch 'main':
    bpo-43853: Handle sqlite3_value_text() errors (GH-25422)
    006fd86

    @miss-islington
    Copy link
    Contributor

    New changeset 067d6d4 by Miss Islington (bot) in branch '3.10':
    bpo-43853: Handle sqlite3_value_text() errors (GH-25422)
    067d6d4

    @serhiy-storchaka
    Copy link
    Member

    I think it is worth to backport it to 3.9.

    It looks to me that it fixes also support of strings containing NUL. It would be nice to add corresponding tests for this. You can also add this in the NEWS entry for this issue.

    @serhiy-storchaka serhiy-storchaka added 3.9 only security fixes 3.11 bug and security fixes labels Aug 6, 2021
    @serhiy-storchaka serhiy-storchaka added 3.9 only security fixes 3.11 bug and security fixes labels Aug 6, 2021
    @erlend-aasland
    Copy link
    Contributor Author

    I'll add extra tests in a separate PR, so we can easily backport it to 3.10 and 3.9.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 8c07fef by Erlend Egeberg Aasland in branch '3.9':
    [3.9] bpo-43853: Handle sqlite3_value_text() errors (GH-25422). (GH-27627)
    8c07fef

    @serhiy-storchaka
    Copy link
    Member

    What about new tests and NEWS update?

    @erlend-aasland
    Copy link
    Contributor Author

    What about new tests and NEWS update?

    See PR 27642 for the expanded tests, and PR 27922 for amending news entries. I didn't combine the two, as I figured you might have more comments on the former PR.

    @pablogsal
    Copy link
    Member

    New changeset 7903a10 by Erlend Egeberg Aasland in branch 'main':
    bpo-43853: Amend NEWS entry for latest changes in sqlite3 (GH-27922)
    7903a10

    @pablogsal
    Copy link
    Member

    New changeset 0ec17a2 by Erlend Egeberg Aasland in branch '3.9':
    [3.9] bpo-43853: Amend NEWS entry for latest changes in sqlite3 (GH-27922) (GH-27952)
    0ec17a2

    @pablogsal
    Copy link
    Member

    New changeset b34ca7e by Erlend Egeberg Aasland in branch '3.10':
    [3.10] bpo-43853: Amend NEWS entry for latest changes in sqlite3 (GH-27922). (GH-27953)
    b34ca7e

    @erlend-aasland
    Copy link
    Contributor Author

    Thanks for merging the NEWS amendments, Pablo. We can close this issue after landing PR 27642.

    @erlend-aasland
    Copy link
    Contributor Author

    Serhiy, I believe I've addressed your review remarks on PR 27642. Would you mind taking a look at it again if you have time?

    @gvanrossum
    Copy link
    Member

    New changeset 3eb3b4f by Erlend Egeberg Aasland in branch 'main':
    bpo-43853: Expand test suite for SQLite UDF's (GH-27642)
    3eb3b4f

    @JelleZijlstra
    Copy link
    Member

    New changeset ba457fe by Erlend Egeberg Aasland in branch '3.10':
    [3.10] bpo-43853: Expand test suite for SQLite UDF's (GH-27642) (GH-31030)
    ba457fe

    @miss-islington
    Copy link
    Contributor

    New changeset 3ea2a8f by Erlend Egeberg Aasland in branch '3.9':
    [3.9] bpo-43853: Expand test suite for SQLite UDF's (GH-27642) (GH-31030) (GH-31586)
    3ea2a8f

    @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
    3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants