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

PyList_SET_ITEM could be safer #74644

Closed
espie mannequin opened this issue May 24, 2017 · 23 comments
Closed

PyList_SET_ITEM could be safer #74644

espie mannequin opened this issue May 24, 2017 · 23 comments
Labels
3.10 interpreter-core type-feature

Comments

@espie
Copy link
Mannequin

@espie espie mannequin commented May 24, 2017

BPO 30459
Nosy @rhettinger, @vstinner, @encukou, @skrah, @serhiy-storchaka, @ctismer, @ZackerySpytz
PRs
  • #19975
  • #23645
  • #23654
  • #28982
  • 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 2020-12-07.10:57:40.893>
    created_at = <Date 2017-05-24.15:14:12.544>
    labels = ['interpreter-core', 'type-feature', '3.10']
    title = 'PyList_SET_ITEM  could be safer'
    updated_at = <Date 2021-10-15.17:44:47.694>
    user = 'https://bugs.python.org/espie'

    bugs.python.org fields:

    activity = <Date 2021-10-15.17:44:47.694>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-12-07.10:57:40.893>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2017-05-24.15:14:12.544>
    creator = 'espie'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30459
    keywords = ['patch']
    message_count = 23.0
    messages = ['294362', '294421', '294450', '294451', '294472', '294490', '294499', '294502', '294507', '294511', '382550', '382560', '382561', '382562', '382626', '382627', '382825', '382830', '382831', '382832', '382971', '384058', '404040']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'vstinner', 'petr.viktorin', 'skrah', 'serhiy.storchaka', 'Christian.Tismer', 'espie', 'ZackerySpytz']
    pr_nums = ['19975', '23645', '23654', '28982']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue30459'
    versions = ['Python 3.10']

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented May 24, 2017

    Documentation says PyList_SET_ITEM is void, but it lies. The macro is such that it yields the actual element being set.

    wrapping the macro content in a do {} while (0)  makes sure PyList_SET_ITEM is really void, e.g.:
    #define PyList_SET_ITEM(op, i, v) do { (((PyListObject *)(op))->ob_item[i] = (v)); } while (0)

    I just ran into the problem while compiling py-qt4 with clang.
    There was some confusion between PyList_SET_ITEM and PyList_SetItem:

    if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)

    g++ didn't catch it (because it doesn't see negative pointers as a problem), but clang++ instantly broke.

    With PyList_SET_ITEM truly void the problem disappears.

    @espie espie mannequin added interpreter-core type-feature labels May 24, 2017
    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 25, 2017

    The docs do make the claim of returning void: https://docs.python.org/3/c-api/list.html#c.PyList_SET_ITEM

    However, I think we should change the docs rather than changing the macro. This macro is very old and very widely used. Changing it is likely to break existing code which may rely on the current behavior.

    Also, I don't want to encourage code like what you saw in py-qt4. It is very indirect about what it is trying to express.

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented May 25, 2017

    Well, there is not going to be a lot of breakage. This problem is the only instance I've encountered in the full OpenBSD ports tree.

    I thought python was supposed to be a clean language, and didn't shy away from removing stuff/tweaking stuff to achieve that goal ?...

    The py-kde4 error was deadly. I'm lucky clang finally caught it, but I'd rather this kind of stuff just not compile.

    I think we're in a world where *correctness* is finally beginning to matter a bit more than *compatibility forever whatever the cost*.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 25, 2017

    I think it would be safer to just cast the result of the expression to void. This decreases the chance of breaking valid code, that for example uses PyList_SET_ITEM() in a comma expression.

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented May 25, 2017

    yep, casting to (void) would be safer indeed. didn't think of that one ;)

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 25, 2017

    Well, there is not going to be a lot of breakage. This
    problem is the only instance I've encountered in the
    full OpenBSD ports tree.

    That is no basis for knowing what the entire rest of the world has done with done with this API (perhaps valid working code using chained assignments).

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented May 25, 2017

    Note that the API is fully documented for returning void... not anything else.

    "No basis" right. We're taling 10000 pieces of software. a lot of what is actually used in the world.

    I'm very surprised, considering python has routinely done "spring cleanup" by breaking fairly old apis.

    If this breaks, people will fix their code, seriously.

    In most places, we would rather have undocumented, unportable code, break *cleanly*, rather than rely on a fuzzy behavior that could possibly change at any moment, and that was never documented as doing anything.

    Or is there some kind of mystique that, because this is low-level C implementation, somehow, python programmers are not going to be able to cope with the internals ?....

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented May 25, 2017

    I guess since it's documented, it would be ok to change for 3.7. BTW, we also allow "static inline" now in headers, so perhaps we could make it a function.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented May 25, 2017

    I prefer to leave this as macro rather than assuming the compiler will heed the inline hint.

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented May 25, 2017

    it's still 100% safe as a macro since each parameter is not used more than once. only the return type is an issue.

    @ZackerySpytz ZackerySpytz mannequin added 3.9 and removed 3.7 labels May 7, 2020
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 5, 2020

    I propose to merge my PR 23645 change right now. If it breaks too many C extensions, we still have time before Python 3.10.0 final scheduled at Monday, 2021-10-04, to revert the change.

    "if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)"

    Well, this is obviously a bug in py-qt4 and it's a good thing to get a compiler error on such code. I guess that the intent was to use PyList_SetItem().

    It's also an issue for Python implementation other than CPython which does not behave exactly like CPython C API. I prefer to fix the C API to make it respect its own documentation (return "void").

    Espie Marc: "Well, there is not going to be a lot of breakage. This problem is the only instance I've encountered in the full OpenBSD ports tree."

    Oh, thanks for providing this very useful feedback! That's very promising.

    Did you report the issue to py-qt4 upstream? If yes, can you please copy here the link to your report?

    @espie
    Copy link
    Mannequin Author

    @espie espie mannequin commented Dec 5, 2020

    On Sat, Dec 05, 2020 at 01:28:33AM +0000, STINNER Victor wrote:

    STINNER Victor <vstinner@python.org> added the comment:

    I propose to merge my PR 23645 change right now. If it breaks too many C extensions, we still have time before Python 3.10.0 final scheduled at Monday, 2021-10-04, to revert the change.

    "if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)"

    Well, this is obviously a bug in py-qt4 and it's a good thing to get a compiler error on such code. I guess that the intent was to use PyList_SetItem().

    It's also an issue for Python implementation other than CPython which does not behave exactly like CPython C API. I prefer to fix the C API to make it respect its own documentation (return "void").

    Espie Marc: "Well, there is not going to be a lot of breakage. This problem is the only instance I've encountered in the full OpenBSD ports tree."

    Oh, thanks for providing this very useful feedback! That's very promising.

    Did you report the issue to py-qt4 upstream? If yes, can you please copy here the link to your report?

    I'm sorry, it's been so long ago, I can't remember.

    I've been dealing with 10s of other bugs since.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 5, 2020

    I prefer PR 19975 to PR 23645. It solves the initial issue and is much simpler.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 5, 2020

    New changeset 556d97f by Zackery Spytz in branch 'master':
    bpo-30459: Cast the result of PyList_SET_ITEM() to void (GH-19975)
    556d97f

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 7, 2020

    New changeset 0ef96c2 by Victor Stinner in branch 'master':
    bpo-30459: Cast the result of PyCell_SET to void (GH-23654)
    0ef96c2

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 7, 2020

    Thanks Espie Marc for the bug report, it's now fixed in the master branch. IMO not only clang users will benefit of a better defined API. For example, it should help other Python implementation to implement such API, without the weird side effects of a macro.

    @vstinner vstinner added 3.10 and removed 3.9 labels Dec 7, 2020
    @vstinner vstinner closed this as completed Dec 7, 2020
    @encukou
    Copy link
    Member

    @encukou encukou commented Dec 10, 2020

    This change goes directly against PEP-387.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 10, 2020

    This change goes directly against PEP-387.

    The change respects the documentation which always documented the result type as "void".

    3.10: https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SET_ITEM
    3.5: https://docs.python.org/3.5/c-api/tuple.html#c.PyTuple_SET_ITEM
    2.7: https://docs.python.org/2.7/c-api/tuple.html#c.PyTuple_SET_ITEM

    This change is backward incompatible on purpose: it's to implement the documented behavior, and the macro was misused leading to a bug: "PyList_SET_ITEM (l, i, obj) < 0" in py-qt4.

    I would also prefer a deprecation warning, but I don't see how do detect when a macro is abused to write "x = SET();" or "SET() = x;" Maybe a C linter could detect that, but I don't know any tool doing that.

    The best we can do is to announce the change. That's why I documented in What's New in Python 3.10, and not only "hidden" in the Changelog:
    https://docs.python.org/dev/whatsnew/3.10.html#id2

    My expectation is that apart py-qt4, no project abuse these 3 macros.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 10, 2020

    I checked: commit 0ef96c2 is part of Python 3.10.0a3 (released 3 days ago).

    @encukou
    Copy link
    Member

    @encukou encukou commented Dec 10, 2020

    The change respects the documentation which always documented the result type as "void".

    Then, IMO, the documentation should be fixed.

    My expectation is that apart py-qt4, no project abuse these 3 macros.

    That's not true; at least ALSA's python bindings use PyTuple_SET_ITEM as a lvalue as well.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 14, 2020

    That's not true; at least ALSA's python bindings use PyTuple_SET_ITEM as a lvalue as well.

    alsa-python used PyTuple_SET_ITEM(..., obj) to decide if it should call Py_INCREF(obj). This code looks suspicious. PyTuple_SET_ITEM() should not be used to set an item to NULL.

    It's already fixed:
    alsa-project/alsa-python@5ea2f87

    IMO it's a good thing that such suspicious code is discovered. The surprising part is that it worked previously :-)

    Downstream Fedora issue: https://bugzilla.redhat.com/show_bug.cgi?id=1906380 (CLOSED)

    @ctismer
    Copy link
    Contributor

    @ctismer ctismer commented Dec 30, 2020

    Congrats to that change!

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Oct 15, 2021

    New changeset 51f8196 by Victor Stinner in branch 'main':
    bpo-30459: Use (void) in macros setting variables (GH-28982)
    51f8196

    @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.10 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants